-
Notifications
You must be signed in to change notification settings - Fork 259
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
base: master
Are you sure you want to change the base?
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 think having the docstrings in two places sounds like a bad idea. This would require some CI checks to test if they are the same etc and would make future PRs complicated. If the current docstrings are not shown in IDEs (I haven't checked that) then one could argue to simply move them over to the stub files all together, but probably in a future PR. I guess it is up to the maintainers to decide such things. |
I agree completely that they should not be in two places in the netCDF4-python repo. That's why I settled on the idea that the script should be run by users post-install, and only if desired. Even that does present the risk that a user might not re-run the script on updating netCDF4 and have outdated docstrings in their IDE, but that seems fairly low-risk for the benefit (IMO).
No, I don't think so, because then the docstrings would be lost from the runtime and, therefore, the REPL. As far as I can tell, the docstrings do belong in the source. If, down the road, the API and Cython implementation were ever split, then all the doctrings and annotations could live in the API ( |
I would say, we focus on typing first in this PR and then open a separate issue for the docstrings. |
Variable typeI'd like to address the question of I think there are some questions we need to try to answer:
I did some research and got this table of results:
The following cannot be used as
Based on all of the above, I propose the following truncated implementation: Edited to add: |
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 recognize that I have included a ton of stuff in this review. I've only reviewed the stubs so I don't know if/how any changes will affect tests etc. I'm going to be offline for the next few weeks so feel free to use or ignore whatever you want from this review.
I will add, I didn't have any problems with my stub file working under Python 3.8, in fact I developed it under 3.8. I did have to get |
No reason, just an oversight on my part. OK to fix. |
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 |
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