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

Include the explicit type in ColorLike and RectLike #3183

Merged
merged 2 commits into from
Oct 19, 2024

Conversation

aatle
Copy link
Contributor

@aatle aatle commented Oct 18, 2024

Add the concrete classes to the ColorLike and RectLike type unions.
They are already implied by the other types, but should be added for clarity.

Since they are only type aliases to unions, their variable name is not always given, so what they represent might not be entirely obvious to users.

Bonus: Sort types in ColorLike by how often they are used.

Example mypy error message, before:

$ mypy test.py
test.py:38: error: Incompatible types in assignment (expression has type "float", variable has type "int | str | SequenceLike[int]")  [assignment]
test.py:39: error: Incompatible types in assignment (expression has type "int", variable has type "SequenceLike[float] | SequenceLike[SequenceLike[float]] | _HasRectAttribute")  [assignment]

After (much more clear!):

$ mypy test.py
test.py:38: error: Incompatible types in assignment (expression has type "float", variable has type "Color | SequenceLike[int] | str | int")  [assignment]
test.py:39: error: Incompatible types in assignment (expression has type "int", variable has type "Rect | FRect | SequenceLike[float] | SequenceLike[SequenceLike[float]] | _HasRectAttribute")  [assignment]

Note:
typing.py now imports things from pygame. I don't think there is any risk of circular imports though.

Note 2:
The _<Shape>Like type unions in geometry.pyi include their concrete classes already.

@aatle aatle requested a review from a team as a code owner October 18, 2024 03:53
@yunline yunline added type hints Type hint checking related tasks typing pygame.typing labels Oct 18, 2024
Copy link
Member

@damusss damusss left a comment

Choose a reason for hiding this comment

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

No, there shouldn't be circular imports.
I have nothing against it, it's redundant but it's clearer, so why not.

Copy link
Member

@ankith26 ankith26 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 not 100% onboard making verbose unions more verbose, but I can see your argument for these changes. Either way, this PR is not doing anything incorrect so I am approving anyways. Thanks! 🎉

@ankith26 ankith26 added this to the 2.5.3 milestone Oct 19, 2024
@ankith26
Copy link
Member

I'll just leave this PR open for a bit more before merging incase anyone else wants to chime in

Copy link
Contributor

@bilhox bilhox left a comment

Choose a reason for hiding this comment

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

LGTM ! The changes seem fair to me.

@bilhox bilhox merged commit 8b09b1f into pygame-community:main Oct 19, 2024
27 checks passed
@aatle aatle deleted the union-explicit-type branch October 19, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type hints Type hint checking related tasks typing pygame.typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants