Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix type hints for undeprecated contour APIs#27217
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
lib/matplotlib/contour.pyi Outdated
| def allkinds(self) -> list[np.ndarray]: ... | ||
| @property | ||
| def allsegs(self) -> list[np.ndarray]: ... |
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.
If I've read these correctly, these are lists of lists of arrays:
| defallkinds(self)->list[np.ndarray]: ... | |
| @property | |
| defallsegs(self)->list[np.ndarray]: ... | |
| defallkinds(self)->list[list[np.ndarray]]: ... | |
| @property | |
| defallsegs(self)->list[list[np.ndarray]]: ... |
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.
Yes, and technically the source of these isPath.codes andPath.vertices, which actually are only typed asArrayLike... and the former can actually just beNone (though that is not reflected by the type hint) as well (which I saw when I just ran it to be sure it was actually nested lists)
It is one of these "in practice in this usage the typewill be ndarray or None", but that is relying on knowledge outside of the type system... so I suspect if we were to inline the type hints it would cause a clash...
I also think that theNone behavior is not super intentional in this context... There is a note in thePath docstring that says "don't use codes directly, useiter_segments so that you don't get the Nones from codes"
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.
Only the empty path Ithink willactually havecodes=None... but that is in fact a case covered by the code...
Noticed that |
They whad been in the limbo land of deprecated APIs that I didn't bother to ensure were the _best_ type hints because they were deprecated. So I just added them to the allowlist for stubtest, but now that they are undeprecated, make sure that they are correct. While I'm here, may as well get rid of the allow list entries for all of them, even the ones that are still deprecated, as that is documented standard practice going forward that pyi match implementation for deprecations as well
Yes, good catch. |
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'm glad I am not the only one confused bytcolors.
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free tosuggest an improvement. |
… APIs(cherry picked from commit518b268)
Backport PR#27217 on branch v3.8.x: Fix type hints for undeprecated contour APIs
PR summary
They had been in the limbo land of deprecated APIs that I didn't bother to ensure were thebest type
hints because they were deprecated.
So I just added them to the allowlist for stubtest, but now that they are undeprecated,
make sure that they are correct.
While I'm here, may as well get rid of the allow list entries for all of them,
even the ones that are still deprecated, as that is documented standard practice going
forward that pyi match implementation for deprecations as well
PR checklist