Remove property accessors and None initialization of Sprite.rect, Sprite.image
#3572
Conversation
📝 WalkthroughWalkthroughPublic sprite typings tightened in stubs (image/rect made non-Optional; DirtySprite.source_rect made Optional). Runtime Sprite API changed from property-backed private storage to public attributes Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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 (3)
src_py/sprite.py (1)
294-294: Annotate DirtySprite.source_rect at runtime to match stubsAdd a concrete runtime type to aid tooling without reintroducing Optional elsewhere.
- self.source_rect = None + self.source_rect: Union[Rect, FRect, None] = Nonebuildconfig/stubs/pygame/sprite.pyi (2)
46-53: Prefer attributes over properties in stubs to mirror runtimeRuntime Sprite now exposes plain attributes; stubs should reflect that for accuracy and simpler tooling.
class Sprite(_HasImageAndRect): - @property - def image(self) -> Surface: ... - @image.setter - def image(self, value: Surface) -> None: ... - @property - def rect(self) -> Union[FRect, Rect]: ... - @rect.setter - def rect(self, value: Union[FRect, Rect]) -> None: ... + image: Surface + rect: Union[FRect, Rect]
153-154: get_clip return type should be OptionalLayeredDirty.get_clip can return None until set; adjust stub to avoid type mismatches.
- def get_clip(self) -> Union[FRect, Rect]: ... + def get_clip(self) -> Optional[Union[FRect, Rect]]: ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
buildconfig/stubs/pygame/sprite.pyi(3 hunks)src_py/sprite.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src_py/sprite.py (2)
buildconfig/stubs/pygame/sprite.pyi (7)
mask(43-43)rect(33-33)rect(51-51)rect(53-53)image(38-38)image(47-47)image(49-49)buildconfig/stubs/pygame/rect.pyi (2)
FRect(304-304)Rect(303-303)
buildconfig/stubs/pygame/sprite.pyi (2)
buildconfig/stubs/pygame/rect.pyi (2)
FRect(304-304)Rect(303-303)buildconfig/stubs/pygame/surface.pyi (1)
Surface(42-1051)
⏰ 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). (20)
- GitHub Check: arm64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
🔇 Additional comments (5)
src_py/sprite.py (3)
88-88: Typing import change is fineUsing Union keeps compatibility with supported Python versions.
93-94: Imports aligned with new annotationsRect/FRect/Surface imports match usage.
113-115: Restoring public attributes for image/rect looks goodMatches upstream expectations and removes accessor overhead.
buildconfig/stubs/pygame/sprite.pyi (2)
31-39: Non-optional rect/image in Protocols — LGTMThese tighten contracts for collision/draw helpers appropriately.
71-71: DirtySprite.source_rect -> Optional — LGTMMatches runtime default None and LayeredDirty usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
buildconfig/stubs/pygame/sprite.pyi (3)
32-34: Non-Optional rect is correct; prefer attribute over property in the protocol.Tightening to Union[FRect, Rect] matches runtime expectations. To better align with structural typing and the runtime (which exposes a plain attribute), consider declaring rect as a plain attribute on the protocol instead of a read-only property.
Apply:
class _HasRect(Protocol): - @property - def rect(self) -> Union[FRect, Rect]: ... + rect: Union[FRect, Rect]
37-39: Non-Optional image is correct; prefer attribute over property in the protocol.As above, switching to a simple attribute on the protocol avoids unnecessary read-only semantics and matches the runtime Sprite API.
Apply:
class _HasImageAndRect(_HasRect, Protocol): - @property - def image(self) -> Surface: ... + image: Surface
46-47: Minor DRY: introduce a local alias for Rect/FRect.You repeat Union[FRect, Rect] several times. A small alias improves readability and future edits.
Apply (add alias near existing imports), then use it on these lines:
@@ -from pygame.rect import FRect, Rect +from pygame.rect import FRect, Rect +Rectish = Union[FRect, Rect] @@ - rect: Union[FRect, Rect] # Same as image, see above + rect: Rectish # Same as image, see above @@ - source_rect: Optional[Union[FRect, Rect]] + source_rect: Optional[Rectish]Also applies to: 65-65
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
buildconfig/stubs/pygame/sprite.pyi(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
buildconfig/stubs/pygame/sprite.pyi (4)
buildconfig/stubs/pygame/rect.pyi (2)
FRect(304-304)Rect(303-303)buildconfig/stubs/pygame/surface.pyi (1)
Surface(42-1051)buildconfig/stubs/pygame/mask.pyi (1)
Mask(18-60)src_py/sprite.py (1)
Sprite(98-250)
⏰ 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). (5)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-latest)
🔇 Additional comments (2)
buildconfig/stubs/pygame/sprite.pyi (2)
65-65: DirtySprite.source_rect now Optional: LGTM.Matches typical usage where None signifies a full redraw region. No further changes needed.
46-47: Verify stubtest for Sprite.image/rect on Python 3.8–3.13
No@propertyaccessors exist at runtime, so the stub’s attribute annotations are correct; runmypy.stubtest(or Pyright’s stub test) against Python 3.8, 3.9, 3.10, 3.11, 3.12, and 3.13 to confirm no false positives on missing instance attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src_py/sprite.py (2)
88-88: Avoid importing pygame.surface at import-time; use TYPE_CHECKING + quoted annotation.Prevents importing the C extension on module import and keeps runtime lean. Safe because Surface is only used for typing here.
-from typing import Union +from typing import TYPE_CHECKING, Union @@ -from pygame.surface import Surface +if TYPE_CHECKING: + from pygame.surface import Surface @@ - image: Surface + image: "Surface"Also applies to: 93-95, 113-113
114-114: Use PEP 604 unions when supported.If minimum Python is 3.10+, this is cleaner.
- rect: Union[Rect, FRect] + rect: Rect | FRect
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
buildconfig/stubs/pygame/sprite.pyi(3 hunks)src_py/sprite.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buildconfig/stubs/pygame/sprite.pyi
🧰 Additional context used
🧬 Code graph analysis (1)
src_py/sprite.py (3)
src_py/__init__.py (1)
Surface(227-228)buildconfig/stubs/pygame/sprite.pyi (3)
mask(43-43)rect(33-33)image(38-38)buildconfig/stubs/pygame/rect.pyi (2)
FRect(304-304)Rect(303-303)
⏰ 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). (18)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: dev-check
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-14)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: i686
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: x86
- GitHub Check: AMD64
🔇 Additional comments (2)
src_py/sprite.py (2)
113-115: Direct attributes for image/rect: LGTM.This aligns with upstream pygame, removes property overhead, and tightens typing.
1326-1326: Docstring fix matches behavior.Returning “Rect or None” reflects self._clip initialization and usage.
aatle
changed the title
None initialization of Sprite.rect and Sprite.image, and remove property accessors None initialization of Sprite.rect, Sprite.image
Revert the changes of #1942 (which initializes
rectandimagetoNone, and turns those attributes into properties).Reasons:
The
Nonevalues aren't actually compatible with the sprite functions and methods; they will error for aNonespriterectorimage. It isn't really useful to initialize them toNone.By having them uninitialized instead, the remaining sprite stub issues involving optionals can be solved. -> makes sprite module comfortably usable and safe with typing.
The property accessors for these attributes seem to be redundant. Also, they are slightly slower than normal attributes.
The revert will also make sprite behavior in line with upstream
pygame, as #1942 didn't apply to it.Additionally, this PR fixes typing for
DirtySprite.source_rectandLayeredDirty.get_clip().