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

Add nwm_client documentation and minor subpackage level import changes #179

Merged
merged 11 commits into from Mar 3, 2022

Conversation

aaraney
Copy link
Member

@aaraney aaraney commented Feb 10, 2022

Update / add too / fix issues with nwm_client readme and docstrings. Adds usage example for gcp client to readme.

Additions

  • nwm_client submodule members, gcp and http can now be accessed from the subpackage level.

Checklist

  • PR has an informative and human-readable title
  • PR is well outlined and documented. See #12 for an example
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (see CONTRIBUTING.md)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output) using numpy docstring formatting
  • Placeholder code is flagged / future todos are captured in comments
  • Reviewers requested with the Reviewers tool ➡️

Copy link
Collaborator

@jarq6c jarq6c left a comment

Choose a reason for hiding this comment

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

Appreciate the updated docs! Seems non-controversial. Just need some clarification on __init__.py imports (in terms of best practices/guiding philosophy/Pythonic-ness).

@@ -1,2 +1,13 @@
# removing __version__ import will cause build to fail. see: https://github.com/pypa/setuptools/issues/1724#issuecomment-627241822
from ._version import __version__

from . import http
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a good idea?

Copy link
Collaborator

Choose a reason for hiding this comment

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

By which I mean, we're not "polluting the namespace", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it's a matter of opinion and style. Personally, given our audience, I would like to make the experience using hydrotools as intuitive as possible. Here, I am assuming it is more intuitive to a user to import nwm_client and then tab complete to see the available submodules. Ex:

from hydrotools import nwm_client

# tab to see exposed subpackage level entities 
nwm_client. # tab tab

nwm_client.html, nwm_client.gcp 

To be fair, I am making a fair number of assumptions about how someone might use hydrotools. What do you think about my assumptions? To your point regarding clouding the namespace, im not sure im ready to answer that. I need to think about it more and consider what implications my assumptions might have.

Copy link
Collaborator

@jarq6c jarq6c Feb 25, 2022

Choose a reason for hiding this comment

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

Assuming you've mulled over the implications, let's see how it works out. No need for the PR to go stale because of this minor concern.

@jarq6c
Copy link
Collaborator

jarq6c commented Feb 11, 2022

Link to #178
Link to #170
?

@jarq6c jarq6c merged commit 3a490cb into NOAA-OWP:main Mar 3, 2022
@aaraney aaraney deleted the add_to_nwm_documentation branch May 19, 2023 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants