-
Notifications
You must be signed in to change notification settings - Fork 263
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 static typing support #1302
Conversation
Thanks! Looks good - but why did you add a new workflow to run stubtest, instead of adding the extra test to the existing workflows? |
Good question. I just took the changes from the previous PR #1279 After the holidays I can continue to work on this and use the existing workflows. |
Looks you like need to add |
I was the author of #1279 , something went wrong with my pr and when I noticed this one was already made. Which is perfectly fine but now this pr has been dead for 5 months. |
Hey, I was just very busy the last months. And thanks for your initial work, it was very helpful to get started. |
I would say I leave it like this. The Variable proposal with all the Descriptors looks quite hacky and is beyond my knowledge. |
stubtest is failing |
Can you try again please? |
still failing |
waiting for @RandallPittmanOrSt to complete his review |
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.
Sorry, I didn't mean to leave this hanging. I'm fine with going ahead with this PR as-is.
@headtr1ck what is the procedure for updating this when a new function is added to the API (for example, PR #1348)? |
New or changed API has to be manually added to the stub files. It should be easier directly after writing the code for it, then what we did now... The stub test should fail if this gets forgotten, so not much intervention required from reviewers/maintainers. Speaking of, maybe we should add this to some contributing guidelines? |
@headtr1ck good idea about adding a CONTRIBUTING.md file - but that can wait for a separate PR. Merging now... |
Hey @jswhit, just curious when this might be available in a release? From the github UI it looks like it hasn't been released (understandable). I just ran into this where my IDE and mypy got mad at me for typing with netCDF4 classes that it didn't understand. |
Closes: #1298
Lots of this PR was stolen from #1279
Notice: I intentionally reversed the import behavior of
__init__.py
and_netCDF4.pyx
in the stubs. This should be more in line in how people are using netCDF4. And while doing so I also removed the "._netCDF4" part of the classname in the reprs.The current typing is working with python 3.11.
TODO:
Make typing pass with python 3.8 (aka. minimum supported version)Won't do