Clean Surface docs 2: Rewrite object, convert, flags docstrings #3604
Conversation
📝 WalkthroughWalkthroughRewrote and expanded the Surface stub docstrings in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildconfig/stubs/pygame/surface.pyi(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
buildconfig/stubs/pygame/surface.pyi (1)
src_py/__init__.py (1)
Surface(227-228)
🔇 Additional comments (4)
buildconfig/stubs/pygame/surface.pyi (4)
49-89: Excellent documentation improvements.The rewritten Surface class docstring significantly improves clarity:
- Clear explanation of Surface creation behavior (lines 49-53)
- Well-structured enumeration of format control methods (lines 55-66)
- Comprehensive coverage of transparency types (lines 78-89)
- Helpful guidance on pixel access and performance (lines 99-114)
The new structure makes the documentation much more accessible for users.
Also applies to: 94-115
289-315: Clear and comprehensive convert() documentation.The rewritten docstring effectively explains the multiple ways to control pixel format conversion:
- No-argument form (display format matching)
- Surface-based format copying
- Bit depth and flags specification
- Bitmask-based control
The documentation of the
pygame.errorexception (lines 298-299, 327) when the display Surface is uninitialized is a valuable addition for users.Consider verifying the
pygame.errorbehavior by checking the actual implementation or testing, though this appears consistent with standard pygame behavior.
317-330: Improved convert_alpha() clarity.The updated documentation better explains the optimization trade-off: the resulting surface format is tailored for fast alpha blitting rather than exactly matching the display format. This is an important distinction for users to understand.
797-813: Well-organized flag documentation.The restructured flag listing improves usability:
- Clear enumeration of current flags with descriptions (lines 798-804)
- Explicit reference to display-specific flags (lines 806-807)
- Separated obsolete flags section for historical reference (lines 809-812)
This organization makes it easier for users to understand which flags are relevant for their use cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
buildconfig/stubs/pygame/surface.pyi (3)
55-55: Minor grammar improvement.The phrase "is providing" could be more idiomatic as "is by providing" or "is to provide".
Apply this diff:
-One way to control the pixel format/characteristics is providing a bitmask of flags: +One way to control the pixel format/characteristics is by providing a bitmask of flags:
111-111: Simplify awkward formatting.The phrase "lock()'ed" uses awkward formatting. Consider using "locked" for better readability.
Apply this diff:
- Any functions that directly access a Surface's pixel data will need that - Surface to be lock()'ed in some cases. Built in functions that manipulate + Any functions that directly access a Surface's pixel data will need that + Surface to be locked in some cases. Built-in functions that manipulate Surfaces will lock and unlock the Surface automatically. Only use manualNote: Also corrected "Built in" to "Built-in" for consistency.
306-307: Improve informal phrasing.The phrase "calculated wrong" is informal. Use "incorrectly calculated" or "miscalculated" for more professional documentation.
Apply this diff:
- * Passing the bitmasks of the pixels and flags. Note that the bit depth - could be calculated wrong, therefore it is not advised to use this - path as it might be deprecated in the future. + * Passing the bitmasks of the pixels and flags. Note that the bit depth + could be incorrectly calculated, therefore it is not advised to use this + approach as it might be deprecated in the future.Note: Also changed "this path" to "this approach" for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildconfig/stubs/pygame/surface.pyi(4 hunks)
🔇 Additional comments (2)
buildconfig/stubs/pygame/surface.pyi (2)
320-327: LGTM!The
convert_alpha()docstring clearly explains that the result is optimized for alpha blitting rather than matching the display format exactly, and correctly uses "raised" for the exception. The documentation accurately describes the behavior and constraints.
797-813: LGTM!The
get_flags()docstring provides a comprehensive and well-organized list of flags, clearly distinguishing between active, private, and obsolete flags. The explicit examples of obsolete flags help developers understand which features are no longer relevant in pygame 2.
| All types of transparency can be used together and correctly mix, except | ||
| pre-multiplied alpha does not consider colorkey nor global alpha. | ||
| Also, An alpha of 255 is opaque while an alpha of 0 is fully transparent. |
There was a problem hiding this comment.
Fix capitalization error.
The word "An" should be lowercase "an" following a comma.
Apply this diff:
- Also, An alpha of 255 is opaque while an alpha of 0 is fully transparent.
+ Also, an alpha of 255 is opaque while an alpha of 0 is fully transparent.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Also, An alpha of 255 is opaque while an alpha of 0 is fully transparent. | |
| Also, an alpha of 255 is opaque while an alpha of 0 is fully transparent. |
🤖 Prompt for AI Agents
In buildconfig/stubs/pygame/surface.pyi around line 92, fix the capitalization
after the comma: change "An alpha of 255 is opaque while an alpha of 0 is fully
transparent." to use a lowercase "an" following the comma so the sentence reads
"...an alpha of 255 is opaque while an alpha of 0 is fully transparent."; update
the line accordingly and run a quick lint/typo check.
| A :class:`pygame.error` exception is thrown if the display Surface | ||
| was not initialized. |
There was a problem hiding this comment.
Use Python exception terminology.
Python exceptions are "raised" rather than "thrown". This ensures consistency with standard Python documentation conventions.
Apply this diff:
- A :class:`pygame.error` exception is thrown if the display Surface
- was not initialized.
+ A :class:`pygame.error` exception is raised if the display Surface
+ was not initialized.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| A :class:`pygame.error` exception is thrown if the display Surface | |
| was not initialized. | |
| A :class:`pygame.error` exception is raised if the display Surface | |
| was not initialized. |
🤖 Prompt for AI Agents
In buildconfig/stubs/pygame/surface.pyi around lines 298 to 299, the docstring
uses the term "thrown" for exceptions; replace it with Python terminology
"raised" so the sentence reads that "A :class:`pygame.error` exception is raised
if the display Surface was not initialized." Update the line accordingly to
maintain consistency with Python docs.
There was a problem hiding this comment.
You're AI can you not notice this improvements one at a time so I don't spam commits 🙏
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other instances of "thrown" in the surface.pyi file
rg -n "thrown" buildconfig/stubs/pygame/surface.pyi
Length of output: 83
@damusss, I understand! I've checked the entire file and this is actually the only instance of "thrown" vs "raised", so there's nothing else to batch with it for now.
For future reviews, I'll note to look for patterns across the entire PR and batch similar improvements together to minimize commits. 👍
🧬 Code Graph Analysis Results
[No. of relevant snippets: 4]
buildconfig/stubs/pygame/mixer.pyi, lines 76-76
def copy(self) -> Self: ...
buildconfig/stubs/pygame/cursors.pyi, lines 83-83
def copy(self) -> Cursor: ...
buildconfig/stubs/pygame/mask.pyi, lines 21-21
def copy(self) -> Mask: ...
buildconfig/stubs/pygame/pixelarray.pyi, lines 19-19
def surface(self) -> Surface: ...
✏️ Learnings added
Learnt from: damusss
PR: pygame-community/pygame-ce#3604
File: buildconfig/stubs/pygame/surface.pyi:298-299
Timestamp: 2025-10-11T10:24:52.698Z
Learning: For the pygame-community/pygame-ce repository, batch similar documentation or terminology improvements together in a single comment rather than pointing them out individually. This helps avoid unnecessary commit spam.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
I'm splitting #3455 . This PR contains the four docstrings that are closely related to surface format internals and that have been rewritten almost entirely, requiring more attention.