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 static typing support #1302

Merged
merged 58 commits into from
Jul 8, 2024
Merged

Add static typing support #1302

merged 58 commits into from
Jul 8, 2024

Conversation

headtr1ck
Copy link
Contributor

@headtr1ck headtr1ck commented Dec 15, 2023

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

@CLAassistant
Copy link

CLAassistant commented Dec 15, 2023

CLA assistant check
All committers have signed the CLA.

@jswhit
Copy link
Collaborator

jswhit commented Dec 29, 2023

Thanks! Looks good - but why did you add a new workflow to run stubtest, instead of adding the extra test to the existing workflows?

@headtr1ck
Copy link
Contributor Author

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.

@jswhit
Copy link
Collaborator

jswhit commented Dec 29, 2023

Looks you like need to add git submodule update --init --recursive after the git clone in stubtest.yml.

@Woefie
Copy link

Woefie commented Jun 6, 2024

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.
The only problem I see now with this pr is that mypy was not installed in the the workflow. Could this just be added and move on?
and merge latest version in this branch

@headtr1ck
Copy link
Contributor Author

Hey, I was just very busy the last months.
I can try to get it to work the next days.

And thanks for your initial work, it was very helpful to get started.

@headtr1ck
Copy link
Contributor Author

I would say I leave it like this. The Variable proposal with all the Descriptors looks quite hacky and is beyond my knowledge.
I propose to merge as is, and @RandallPittmanOrSt can open a follow-up PR to improve things.

@jswhit
Copy link
Collaborator

jswhit commented Jun 27, 2024

stubtest is failing

@headtr1ck
Copy link
Contributor Author

stubtest is failing

Can you try again please?

@jswhit
Copy link
Collaborator

jswhit commented Jun 27, 2024

stubtest is failing

Can you try again please?

still failing

src/netCDF4/_netCDF4.pyx Show resolved Hide resolved
docs/index.html Outdated Show resolved Hide resolved
@jswhit
Copy link
Collaborator

jswhit commented Jul 6, 2024

waiting for @RandallPittmanOrSt to complete his review

Copy link
Contributor

@RandallPittmanOrSt RandallPittmanOrSt left a 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.

@jswhit
Copy link
Collaborator

jswhit commented Jul 7, 2024

@headtr1ck what is the procedure for updating this when a new function is added to the API (for example, PR #1348)?

@headtr1ck
Copy link
Contributor Author

@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?

@jswhit
Copy link
Collaborator

jswhit commented Jul 8, 2024

@headtr1ck good idea about adding a CONTRIBUTING.md file - but that can wait for a separate PR. Merging now...

@jswhit jswhit merged commit 90f3252 into Unidata:master Jul 8, 2024
1 check passed
@djhoese
Copy link

djhoese commented Jul 29, 2024

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.

@headtr1ck headtr1ck deleted the typing branch November 14, 2024 15:51
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.

Add static type hints
7 participants