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

Update pygame.version to not be an autogen file #2537

Merged
merged 3 commits into from
Nov 1, 2023
Merged

Conversation

ankith26
Copy link
Member

The idea here is to simplify buildconfig a tiny bit.

Instead of having pygame.version auto-generated by setup.py on every build (by doing a hacky lookup into C sources), the version information would be exported via pygame.base as a regular python string.

Also improved tests a bit.

@ankith26 ankith26 requested a review from a team as a code owner October 28, 2023 13:48
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

One minor stub thing that could be fixed.

@ankith26
Copy link
Member Author

Do we need Circle in the base namespace? I see Myre is making changes just to fix CI fail, but the actual issue here is that someone previously updated the stubgen file to add Circle to the base namespace but didn't do it in code.

@ankith26
Copy link
Member Author

The CI is still failing, for now I will take the easier approach of excluding the Circle from stubs, and it can be fixed in a future PR

@MyreMylar
Copy link
Member

The CI is still failing, for now I will take the easier approach of excluding the Circle from stubs, and it can be fixed in a future PR

I think I just needed to add:

Circle: type to locals.pyi to finish off there.

But don't mind pulling it out of the base namespace stubgen either.

src_c/base.c Outdated Show resolved Hide resolved
Copy link
Member

@novialriptide novialriptide left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@novialriptide novialriptide merged commit c6b99e8 into main Nov 1, 2023
29 checks passed
@ankith26 ankith26 added this to the 2.4.0 milestone Nov 3, 2023
@ankith26 ankith26 deleted the ankith26-version branch November 3, 2023 14:19
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