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 pygame.typing module #3002

Merged
merged 20 commits into from
Aug 18, 2024
Merged

Conversation

damusss
Copy link
Member

@damusss damusss commented Jul 16, 2024

This PR would potentially implement #1682

As the title says, this PR adds the typing module to pygame. I've made some libraries myself and I often wanted to annotate properly things as colors and rects, but the types they accept are a wide range, especially the rect, so it would be really nice to put rect: pygame.typing.RectLike in an argument for example. I see no reason not to have this.

Details about the PR:

  • only the contents of _common.pyi have been ported. This can be easily expanded in the future.
  • _common.pyi has been removed
  • src_py/typing.py has been added, being similar to _common.pyi
  • gen_stubs.py has the functionality of copying typing.py to typing.pyi, to make type checkers happy
  • __init__.py was modified to correctly import the typing module
  • Since the API will be public, a consistent pattern has been chosen with @ankith26 , which is to add "Like" to the types, to annotate that it's not the actual object but rather a set of types that can replace it (if one needs the object to access the attributes, pygame.Rect is preferred over pygame.typing.RectLike, while if the object is going to be passed to a pygame function that accepts RectLike, there's no point in forcing a pygame.Rect. You get the idea.)

Tests and docs have been added. Docs are currently temporary, but might not be when you reader read this message.

@damusss damusss added the type hints Type hint checking related tasks label Jul 16, 2024
@damusss damusss requested a review from a team as a code owner July 16, 2024 17:32
@damusss damusss changed the title [WIP] Add pygame.typing module [Almost Complete] Add pygame.typing module Jul 17, 2024
src_py/typing.py Show resolved Hide resolved
@Starbuck5
Copy link
Member

So before we add a whole new module for this, we should think about some things.

How do other libraries do this?

Does this need to be a module or can type variables be scattered where they are relevant? Like pygame.rect.RectLike?

How can we keep this stable? This exposes more stuff that we need to keep stable, are we okay with that?

@damusss
Copy link
Member Author

damusss commented Jul 18, 2024

So before we add a whole new module for this, we should think about some things.

How do other libraries do this?

Does this need to be a module or can type variables be scattered where they are relevant? Like pygame.rect.RectLike?

How can we keep this stable? This exposes more stuff that we need to keep stable, are we okay with that?

  1. you could realistically only scatter rect, color and maybe rgba, maybe path and file to rwobject, but those are all implemented in C and they would require some injection.

  2. what non stable things are we exposing?

@Matiiss
Copy link
Member

Matiiss commented Jul 18, 2024

As for how other libraries do it, here's numpy:
https://github.com/numpy/numpy/tree/main/numpy/typing
https://github.com/numpy/numpy/tree/main/numpy/_typing

Note that the _typing package is internally imported in stub (.pyi) files

@damusss damusss added the New API This pull request may need extra debate as it adds a new class or function to pygame label Jul 19, 2024
@damusss damusss changed the title [Almost Complete] Add pygame.typing module Add pygame.typing module Jul 20, 2024
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.

Overall, I really like where this PR is going! I am approving the code changes from my side. I left a few reviews for improving the test file.

As this is a significant change, I would like for this PR to have 3 approving reviews.

buildconfig/stubs/typing_sample_app.py Show resolved Hide resolved
buildconfig/stubs/typing_sample_app.py Show resolved Hide resolved
buildconfig/stubs/stubcheck.py Show resolved Hide resolved
src_py/typing.py Show resolved Hide resolved
@ankith26
Copy link
Member

Addressing some of Starbuck's points (in favour of this PR) from my POV

How do other libraries do this?
Does this need to be a module or can type variables be scattered where they are relevant? Like pygame.rect.RectLike?

As Matt already pointed out, numpy already has a dedicated submodule for typing stuff. In the future, if needed, we can make aliases in scattered places, but I'd like all the implementation to be in one file.

How can we keep this stable? This exposes more stuff that we need to keep stable, are we okay with that?

This PR already adds a test file, that should help with maintaining stability. OFC, the file could be expanded in future iterations.
If this makes approval more comfortable: we can always slap an "experimental" notice on the docs, and allow for more drastic changes here for a few releases.

Copy link
Member

@Matiiss Matiiss left a comment

Choose a reason for hiding this comment

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

LGTM, I sense this will be a great addition to pygame-ce

Maybe the type aliases could be hinted as TypeAliases, but if type checkers don't complain then it's probably that important.

I suppose sometime in the future we could restructure this a bit so that there's no need to literally copy-paste code from typing.py to typing.pyi, but I suppose it should be fine for now.

Thanks! 🚀

@ankith26 ankith26 added this to the 2.5.1 milestone Jul 23, 2024
@ankith26 ankith26 linked an issue Jul 23, 2024 that may be closed by this pull request
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.

Overall LGTM, I left some requested changes for the documentation part.
I feel like for people reading the docs, a list of structure for each type in pygame.type is a lot better for the user. Even though it's abstract and can technically handles an infinite number of type as long as it respects the structure, making a list of the most common types used feels essential for the user.

docs/reST/ref/typing.rst Outdated Show resolved Hide resolved
docs/reST/ref/typing.rst Outdated Show resolved Hide resolved
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.

pygame.gfxdraw does not handle string colors and mapped color, only sequences of 3 or 4 values are accepted (RGB and RGBA).

docs/reST/ref/typing.rst Outdated Show resolved Hide resolved
@damusss
Copy link
Member Author

damusss commented Aug 2, 2024

pygame.gfxdraw does not handle string colors and mapped color, only sequences of 3 or 4 values are accepted (RGB and RGBA).

so should i say supported by "almost" all pygame functions or do you want me to edit gfxdraw.pyi? the typehints were there already and they were wrong before this PR.

@Starbuck5 Starbuck5 modified the milestones: 2.5.1, 2.5.2 Aug 7, 2024
docs/reST/ref/typing.rst Outdated Show resolved Hide resolved
Copy link
Member

@Matiiss Matiiss left a comment

Choose a reason for hiding this comment

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

None of the sprite.pyi types seem to be exposed, is this intended? I find it would be greatly beneficial to expose those types as well. It would help with some testing I plan to do regarding those types and thus it would require access to concrete objects.

@damusss
Copy link
Member Author

damusss commented Aug 13, 2024

None of the sprite.pyi types seem to be exposed, is this intended? I find it would be greatly beneficial to expose those types as well. It would help with some testing I plan to do regarding those types and thus it would require access to concrete objects.

Do you mean types such as _SuportsSprite and _SupportsDirtySprite? It is kinda intended. This PR only implements the contents of common.pyi to keep the scope simple. extra typings can be added later with other PRs based on their need (at the start of the PR I added a lot more types but keeping it small is better). @ankith26 might have a say in the matter.

@Matiiss
Copy link
Member

Matiiss commented Aug 14, 2024

None of the sprite.pyi types seem to be exposed, is this intended? I find it would be greatly beneficial to expose those types as well. It would help with some testing I plan to do regarding those types and thus it would require access to concrete objects.

Do you mean types such as _SuportsSprite and _SupportsDirtySprite? It is kinda intended. This PR only implements the contents of common.pyi to keep the scope simple. extra typings can be added later with other PRs based on their need (at the start of the PR I added a lot more types but keeping it small is better). @ankith26 might have a say in the matter.

Fair enough, it can be handled in a later PR then 👍

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.

This PR still has my approval. This has been some nice work, thanks again for working on this! 😎

I think we can just merge it in for now (as it is a pretty safe change that wouldn't break any code) and see how it goes. If needed we can also mark this as experimental before the next release.

@ankith26 ankith26 merged commit 9208ee5 into pygame-community:main Aug 18, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New API This pull request may need extra debate as it adds a new class or function to pygame type hints Type hint checking related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export types via something like pygame.types (3411)
5 participants