Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve documentation #72

Merged
merged 19 commits into from
Dec 28, 2022
Merged

Conversation

elbeejay
Copy link
Contributor

@elbeejay elbeejay commented Dec 22, 2022

Improvements to documentation and doc-strings

This looks like a large PR, but it mostly consists of cosmetic changes aimed at improving the rendered documentation and standardizing some of the doc-string and code formatting throughout the package. Given the large size, it might be best to spread the review out over a few days (or weeks) and go through changed files individually due to the number of changes. These changes are summarized below:

Closing Issues

Additional Examples

  • Adds at least 1 if not more examples of function usage to the doc-strings of public functions. These will render below the API documentation for a given function, ex:

image

  • Adds (and dynamically renders) the hydroshare notebooks put together by @horsburgh - notebooks were downloaded from https://www.hydroshare.org/resource/c97c32ecf59b4dff90ef013030c54264/ - they do not directly reference the notebooks but reference the downloaded copies in the demos/ subdirectory, it might be possible to pull the latest versions of the notebooks with some fancy CI stuff to download and extract the source notebooks to the proper location every time the documentation workflow is run

Doc-strings and linting

  • PR includes a bunch of edits to standardize the doc-strings to the numpy format. This includes ensuring headers are Parameters, Other Parameters, Returns, and Examples exclusively in the doc-strings, as well as explicit inclusion of parameter types
  • Some linting to conform to standard PEP-standards for spacing in arguments, line lengths, etc.
  • Refer to Python objects in the documentation with double backquotes for Python functions and objects outside of the language, e.g., pandas.DataFrame whereas for functions that are within the package, the convention :obj:object is used, e.g., :obj:dataretrieval.nwis.get_record as Sphinx is able to automatically link to the relevant API documentation in that case
  • Execute much of the documented code, anything preceded by .. doctest:: will be run when the documentation is built, some code is preceded by .. code:: instead because that code is either costly to run (and therefore impractical for the CI job) or requires gdal which is not installed on the CI runner at this time

Code Changes

  • Adds a private function _alter_kwargs to wqp.py to reduce code repetition by using a function to handle the keyword arguments that are not currently supported by dataretrieval
  • Changes the URLs and paths for nadp.py to reflect the current addresses and URL-conventions for the national trends and mercury deposition maps and data

Unit Tests

  • Adds tests for the NADP functions with a nadp_test.py file. These tests are marked to xfail as the CI runner doesn't have gdal installed, but they should pass locally when running the test suite
  • Adds unit tests for the new _alter_kwargs function within the wqp_test.py file

@elbeejay elbeejay added bug Something isn't working documentation Question, request, or suggestion for additional documentation labels Dec 22, 2022
@elbeejay elbeejay marked this pull request as ready for review December 28, 2022 19:50
Copy link
Collaborator

@thodson-usgs thodson-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@@ -119,6 +136,8 @@ def update_merge(left, right, na_only=False, on=None, **kwargs):


class Metadata:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class isn't idiomatic. Note to self to fix.

@thodson-usgs thodson-usgs merged commit 433814f into DOI-USGS:master Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Question, request, or suggestion for additional documentation
Projects
None yet
2 participants