Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

TYP: Fix xycoords and friends #28532

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 16 commits into from
Sep 18, 2024
Merged

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Jul 9, 2024

PR summary

The existing type definition for xycoords (and friends) only support passing a tuple of floats, where in fact a tuple of anything that is valid on its own is acceptable. The easiest way to define this is with type variables, which I then reused everywhere.

I defined this based on the docs here, not really from reading the code, for what it's worth.

PR checklist

@mdboom mdboom marked this pull request as draft July 9, 2024 17:18
@mdboom mdboom marked this pull request as ready for review July 9, 2024 19:11
@tacaswell tacaswell added this to the v3.10.0 milestone Jul 10, 2024
@ksunden
Copy link
Member

ksunden commented Jul 11, 2024

I think the convention is for type aliases/unions to be named like classes (i.e. CamelCase), since they are taking the place of a class as a type hint.

Further, my preference would be to put it with other union definitions in typing.py. (which also has the benefit of not needing to repeat ourselves)

@timhoffm
Copy link
Member

ping @mdboom we plan to cut the 3.10rc in two weeks. It would be nice to get this in. Are you able to address @ksunden's feedback by then? Otherwise, we could take over.

@mdboom
Copy link
Member Author

mdboom commented Sep 16, 2024

Thanks for the ping. I can get to this today.

@mdboom mdboom requested a review from ksunden September 16, 2024 17:48
@mdboom
Copy link
Member Author

mdboom commented Sep 16, 2024

I think I've updated this based on the comments, and it's ready for another look.

@ksunden ksunden merged commit fec6863 into matplotlib:main Sep 18, 2024
39 of 43 checks passed
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.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.