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

Add runtime support for pygame.sprite.AbstractGroup subscripts #3053

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Matiiss
Copy link
Member

@Matiiss Matiiss commented Aug 12, 2024

Currently this code would fail at runtime

import pygame


class MySprite(pygame.sprite.Sprite):
    ...


my_group: pygame.sprite.Group[MySprite] = pygame.sprite.Group()

with

Traceback (most recent call last):
  File "C:\Users\Matiiss\PycharmProjects\Something\for_so.py", line 8, in <module>
    my_group: pygame.sprite.Group[MySprite] = pygame.sprite.Group()
              ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
TypeError: type 'Group' is not subscriptable

Currently a viable alternative is to use

my_group: "pygame.sprite.Group[MySprite]" = pygame.sprite.Group()

or

from __future__ import annotations

However, I feel as though that makes it more cumbersome for the end user.
Also this solves a cross compatibility issue with pg/pg

Besides this, the type stubs for pygame.sprite have to be massively revised, they are quite flawed it seems, especially when this Generic mechanism is taken into account, but I will handle that in a another PR.

@Matiiss Matiiss requested a review from a team as a code owner August 12, 2024 23:22
src_py/sprite.py Outdated Show resolved Hide resolved
src_py/sprite.py Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

LGTM, with this change one doesn't have to wrap the subscript inside a string for annotations.

I don't think importing typing inside the function is a problem because:

  • The class is subscripted a little amount of times
  • Sequential subscripts will use the cached module, so it's just like if you import it at the top of the file

Regarding the generic implementation, I think the sprite stubs do a good enough job with subclassing Generic and using the typevars, so I think the actual implementation can ignore all that and only implement class getitem for the runtime.

Thanks :)

@Starbuck5
Copy link
Member

Now that we've dropped 3.8 is this possible to do without using undocumented features?

@Matiiss Matiiss force-pushed the matiiss-allow-sprite-group-subscripts branch from e0c0698 to 7f3b58f Compare November 24, 2024 13:49
@ankith26 ankith26 linked an issue Nov 27, 2024 that may be closed by this pull request
@Matiiss
Copy link
Member Author

Matiiss commented Nov 28, 2024

I was wondering whether some of this should be put into typing_sample_app.py to ensure that mypy is fine with this, but that file seems more targeted towards typing.py and there already are unit tests for this runtime generic support (though in testing redundancy is fine). Thoughts? Should I add some samples to typing_sample_app.py or is the current testing sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprite pygame.sprite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: type 'Group' is not subscriptable
5 participants