-
Notifications
You must be signed in to change notification settings - Fork 12
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
Use argparse instead of click #45
Conversation
Would you consider removing setup.cfg and setup.py? (I'm not sure what's behind my burning need to erase them.) |
Some tools still use setup.py but I don't think any that we would use do and, I'm pretty sure, the GitHub dependency graph now understands pyproject.toml. So, yeah, we should be able to remove setup.py. As for setup.cfg, I can move the coverage and zest.releaser configuration into pyproject.toml. It'll be a while before we can migrate flake8, as they don't want to support pyproject.toml. Another option would be to move everything from pyproject.toml into setup.cfg. In that case we would only have a single config file. Or we could use something other than flake8, or use it through ruff (but not all of the extensions we use are supported). Things are working well now, though, so I would rather not do that just to eliminate one configuration file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat! I especially like how you made _test
a suffix. Most of my comments are suggestions, but the last two may need fixing.
README.md
Outdated
git clone https://github.com/csdms/bmi-python | ||
cd bmi-python | ||
pip install . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with install from URL?
pip install git+https://github.com/csdms/bmi-python.git
This is a suggestion only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of exactly this but thought you had written this so was going to ask you about it first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the instructions to pip-install from a url as you suggested.
"BMI", | ||
"Basic Model Interface", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the keywords be lowercased? A quick search didn't reveal guidance one way or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea but I think uppercase should be fine.
documentation = "https://bmi.readthedocs.io" | ||
homepage = "https://bmi.readthedocs.io" | ||
repository = "https://github.com/csdms/bmi-python" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that if you cap-case these table entries they get cap-cased on the PyPI project page.
src/bmipy/cmd.py
Outdated
|
||
from bmipy import Bmi | ||
import numpy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the output from bmipy-render
, numpy is referred to as np in several signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included import numpy as np
to satisfy the linters but I don't think it's needed to run the code. Because we import annotations from the future, the type annotations are stringified before the code is executed to enable delayed evaluation.
In the future, we may want to annotate the numpy types with strings to remove the numpy requirement.
src/bmipy/cmd.py
Outdated
|
||
|
||
def render_bmi(name, black=True, hints=True): | ||
def render_bmi(name: str, black: bool = True, hints: bool = True) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type hints in the rendered output have quotes; e.g.,
def finalize(self) -> "None":
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdpiper Could you check this again? I think this should be good now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcflugen Yup, the quotes are gone.
Another thing, though--does NDArray
get imported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdpiper Thanks!
See my comment above about delayed evaluation of annotations. Although I think it's not necessary, for now, I think I'll add NDArray
to the bmi template.
This pull request was originally intended to just replace click with argparse to reduce the number of dependencies but, since the repository hadn't been touched in a bit, I couldn't help myself from tidying things up a bit.
This addresses both #44 and #10.
Highlights:
_test.py
suffix—so much easier for tab-completion and they aren't included in the source dist by default.