Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 28, 2023

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Oct 27, 2023

PR summary

They had 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

PR checklist

@ksunden ksunden added this to the v3.8.1 milestone Oct 27, 2023
Comment on lines 114 to 116
def allkinds(self) -> list[np.ndarray]: ...
@property
def allsegs(self) -> list[np.ndarray]: ...
Copy link
Member

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:

Suggested change
def allkinds(self) -> list[np.ndarray]: ...
@property
def allsegs(self) -> list[np.ndarray]: ...
def allkinds(self) -> list[list[np.ndarray]]: ...
@property
def allsegs(self) -> list[list[np.ndarray]]: ...

Copy link
Member Author

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 is Path.codes and Path.vertices, which actually are only typed as ArrayLike... and the former can actually just be None (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 type will 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 the None behavior is not super intentional in this context... There is a note in the Path docstring that says "don't use codes directly, use iter_segments so that you don't get the Nones from codes"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the empty path I think will actually have codes=None... but that is in fact a case covered by the code...

@ksunden
Copy link
Member Author

ksunden commented Oct 27, 2023

Noticed that tcolors (one of the still deprecated properties) is also missing a layer of nesting, for some reason it is a list of 1-tuples of 4-tuples...

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
@QuLogic
Copy link
Member

QuLogic commented Oct 27, 2023

Yes, good catch.

Copy link
Member

@rcomer rcomer left a 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 by tcolors.

@rcomer rcomer merged commit 518b268 into matplotlib:main Oct 28, 2023
@lumberbot-app
Copy link

lumberbot-app bot commented Oct 28, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v3.8.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 518b268a5e94f8dcec079cb07eb735981e5ac0ef
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #27217: Fix type hints for undeprecated contour APIs'
  1. Push to a named branch:
git push YOURFORK v3.8.x:auto-backport-of-pr-27217-on-v3.8.x
  1. Create a PR against branch v3.8.x, I would have named this PR:

"Backport PR #27217 on branch v3.8.x (Fix type hints for undeprecated contour APIs)"

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 Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

rcomer added a commit to rcomer/matplotlib that referenced this pull request Oct 28, 2023
timhoffm added a commit that referenced this pull request Oct 28, 2023
Backport PR #27217 on branch v3.8.x: Fix type hints for undeprecated contour APIs
@ksunden ksunden mentioned this pull request Nov 2, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants