Clean Surface docs 3: minor clarifications, typos and movements #3605
Conversation
📝 WalkthroughWalkthroughAdds a public read-only Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Surface
Note over Surface: New public read-only\nproperty `_pixels_address`
App->>Surface: read _pixels_address
Surface-->>App: int (pixel buffer start address)
App->>Surface: lock()/unlock()/mustlock()
Surface-->>App: lock state change
App->>Surface: blit()/blits()/fblits()
alt fblits may return None
Surface-->>App: performs blit (returns surfaces or None)
else blits/fblits return list
Surface-->>App: returns blit results list
end
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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
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 |
|
Can you explain why you moved the locking functions / pixel address around? |
This PR just does what I did in the original, so I'll try to explain my past self: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
buildconfig/stubs/pygame/surface.pyi (2)
274-274: Clarify "regular blitting" reference.The phrase "unlike regular blitting" is somewhat vague. Consider specifying "unlike
blit()andblits()" for clarity.Apply this diff:
- :returns: ``None`` (unlike regular blitting) + :returns: ``None`` (unlike `blit()` and `blits()`)
344-346: Consider present tense for consistency.The clarification about subsurface behavior is helpful and accurate. However, for consistency with other docstrings in this file (e.g.,
get_abs_parent()at line 717), consider using present tense.Apply this diff:
- If the Surface was a subsurface, the returned Surface will *not* retain - the parent and will be a regular Surface with its own pixel data. + If this Surface is a subsurface, the returned Surface will *not* retain + the parent and will instead be a regular Surface with its own pixel data.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src_c/doc/surface_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (1)
buildconfig/stubs/pygame/surface.pyi(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: build (macos-14)
- GitHub Check: dev-check
- GitHub Check: aarch64
- GitHub Check: x86
- GitHub Check: AMD64
🔇 Additional comments (5)
buildconfig/stubs/pygame/surface.pyi (5)
153-153: Typo fix looks good.The correction from "blittting" to "blitting" is accurate.
442-450: Documentation improvements look good.The reformatting improves readability, and the version change note provides helpful context.
456-458: Enhanced documentation is clear and helpful.The expanded description of the return value is accurate and provides useful detail about the range and None case.
716-717: Clearer and more precise wording.The clarification improves understanding of the method's behavior in both subsurface and non-subsurface cases.
1047-1054: Verify public API status for underscore-prefixed property.The
_pixels_addressproperty starts with an underscore, which conventionally indicates a private/internal API in Python. However, it's being added to the public stub file and marked as available since version 1.9.2, suggesting it's been part of the public API for a long time.Can you confirm this is intentionally public API despite the naming convention? If so, the addition to the stubs is appropriate. If it's actually private, consider whether it should be exposed in the public stub file.
2 participants
I'm splitting #3455 . This PR handles the remaining small clarifications and typos. The locking functions and the _pixels_address property are also moved at the end of the file with no changes.