Skip to content

_sdl2.video docs rewrite, minor reorderings & method signature changes #2128

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

Merged
merged 2 commits into from
May 21, 2023

Conversation

Mega-JC
Copy link
Member

@Mega-JC Mega-JC commented Apr 23, 2023

(based on #2092)

This commit effectively rewrites all the documentation of the pygame._sdl2.video module, by either adding new information for undocumented content, or extracting information from existing docstrings in the codebase. Along with the documentation changes, some function signatures were modified to adhere to the snake case naming convention described in PEP 8.

@Mega-JC Mega-JC requested a review from a team as a code owner April 23, 2023 23:30
@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch 2 times, most recently from 1daa46e to 99b1862 Compare April 25, 2023 21:30
@Starbuck5 Starbuck5 added docs _sdl2 pygame._sdl2 labels Apr 30, 2023
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Looks good.

Thanks for this!

@Mega-JC Mega-JC changed the title _sdl2.video docs rewrite, minor method signature changes _sdl2.video docs rewrite, code reordering, minor method signature changes May 15, 2023
@Mega-JC Mega-JC changed the title _sdl2.video docs rewrite, code reordering, minor method signature changes _sdl2.video docs rewrite, minor method signature changes May 15, 2023
@Mega-JC Mega-JC changed the title _sdl2.video docs rewrite, minor method signature changes _sdl2.video docs rewrite, minor reorderings & method signature changes May 15, 2023
@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch from 64469b9 to 5d19055 Compare May 15, 2023 23:25
@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch from 93a0a8f to 70984f3 Compare May 17, 2023 15:40
@Mega-JC
Copy link
Member Author

Mega-JC commented May 17, 2023

Alright, I took another good look at the changes made in this PR, and took some time to address all the complaints. Should have done that earlier, but I think it the PR should be ready now.

@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch 3 times, most recently from 818ae99 to d94ecfc Compare May 17, 2023 15:56
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 (apart from that minor thing that's out of the scope of this PR)

EDIT: that minor thing is also now fixed 👍

@Starbuck5 Starbuck5 dismissed yunline’s stale review May 18, 2023 02:32

Changes made.

@Starbuck5
Copy link
Member

This has the approvals to merge, but the cython should be regenerated to sync the default window title change. And to be sure that's safe the branch should be rebased against main to not wipe out any existing cython changes since the inception of this branch.

Also the commit history of the branch is a lot at this point, it could "squash and merge"d or Mega could squash down some commits into each other.

@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch from e4436dc to 2551b7f Compare May 18, 2023 08:19
@Mega-JC
Copy link
Member Author

Mega-JC commented May 18, 2023

All done and completed, Squash and Merge sounds good, now I have to figure out why CI is failing -_-

Edit: Seems like recompilation with Python 3.11 was needed, CI is happy now.

@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch from 2551b7f to 3cf6028 Compare May 18, 2023 09:06
@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch from 3cf6028 to b1b98f1 Compare May 18, 2023 09:26
@Starbuck5 Starbuck5 force-pushed the video-docs-rewrite branch from 3493b92 to 2f75a05 Compare May 20, 2023 06:21
@Starbuck5
Copy link
Member

Edit: Seems like recompilation with Python 3.11 was needed, CI is happy now.

Since Cython is a code generator, the only relevant thing is the version of Cython.

I didn't like how the Cython version changed in your regen, and how every file was regenerated (even ones that shouldn't have any change), so I changed the Cython regen commit a bit.

But now if I squash and merge we'll both be equal authors on this PR, from a commit perspective. Which isn't accurate. I'd like you to squash the first 14 commits into 1 or several commits authored exclusively by you, then we can do a normal merge.

@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch 2 times, most recently from a0ce59c to ef35a86 Compare May 21, 2023 15:25
@Mega-JC Mega-JC force-pushed the video-docs-rewrite branch from ef35a86 to 9b8c23e Compare May 21, 2023 15:25
@Mega-JC
Copy link
Member Author

Mega-JC commented May 21, 2023

Alright, commits have now been squashed down to 2.

@Starbuck5 Starbuck5 merged commit 4153b98 into pygame-community:main May 21, 2023
@Starbuck5 Starbuck5 added this to the 2.3 milestone Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs _sdl2 pygame._sdl2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants