-
-
Notifications
You must be signed in to change notification settings - Fork 194
Improve sprite stubs #3525
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
base: main
Are you sure you want to change the base?
Improve sprite stubs #3525
Conversation
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.
LGTM (though I havent rigorously tested this).
Left a few minor observations but nothing that should block a merge
# must have rect attribute, may optionally have radius attribute | ||
_SupportsCollideCircle = Union[_HasRect, _HasRadiusAndRect] | ||
# Must have rect attribute, may optionally have radius attribute | ||
_SupportsCollideCircle = _HasRect |
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 see that this is a simplification and while its not incorrect to do this, I think keeping this as it is can convey more information to the user? Or is the extra information useless? Im not sure either way
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.
Although it is more information, the redundancy of it made me feel like removing it.
Users could be confused by the redundant union. I was a bit confused too when I realized one was just a subset of the other.
I think the docs are enough in any case.
(Also, sometimes only _SupportsCollideCircle
is seen anyways.)
_TDirtySprite = TypeVar("_TDirtySprite", bound=_SupportsDirtySprite) | ||
_SpriteT = TypeVar("_SpriteT", bound=Sprite) | ||
_SpriteT2 = TypeVar("_SpriteT2", bound=Sprite) | ||
_DirtySpriteT = TypeVar("_DirtySpriteT", bound=DirtySprite) |
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.
This renaming made the diff slightly more complicated to review than it should have been. Is this a standard convention or just preference?
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 believe it is a standard convention: the T
in a typevar name is always a suffix rather than a prefix.
Typing users may see the name of the typevars.
See:
Unfortunately, I did not come up with a shorter name for the typevars.
(Note: I did not change other nonconforming module typevar names.)
Typing fixes, simplifications, improvements, and renames for the sprite module.
Also see #3488, which had other fixes.
In the future, the
Optional
might get removed fromimage
andrect
, possibly with runtime changes. This would fix the last sprite typing issue and make it as comfortable to use static typing with as other pygame modules.