Remove property accessors and None initialization of Sprite.rect, Sprite.image
#3572
Conversation
📝 WalkthroughWalkthroughSprite runtime API changed from property-backed private storage to public attributes Changes
Sequence Diagram(s)sequenceDiagram
participant Code as Caller
participant Sprite as Sprite (object)
rect rgb(240,248,255)
Note over Sprite: Previous (property-backed)
end
Code->>Sprite: access sprite.image (property)
Sprite-->Code: property getter -> internal __image
Code->>Sprite: assign sprite.image = surf
Sprite-->Sprite: property setter -> set __image
rect rgb(245,255,240)
Note over Sprite: New (public attributes)
end
Code->>Sprite: access sprite.image
Sprite-->Code: direct attribute read
Code->>Sprite: assign sprite.image = surf
Sprite-->Sprite: direct attribute write
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-01T20:18:57.500ZApplied to files:
🧬 Code graph analysis (2)src_py/sprite.py (2)
buildconfig/stubs/pygame/sprite.pyi (3)
⏰ 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)
🔇 Additional comments (7)
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: 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
damusss
left a comment
damusss
left a comment
There was a problem hiding this comment.
I don't see issues with it so I will approve it
ankith26
left a comment
ankith26
left a comment
There was a problem hiding this comment.
I would like to request @MyreMylar to review this PR given that this PR is reverting a change they had done.
3 participants
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().