Warn user not to override special magic methods #6625
Conversation
📝 WalkthroughWalkthroughReplaces hard-coded restrictions on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
|
hmm, actually both selectable. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/derive-impl/src/pyclass.rs (1)
915-1000: Document rationale for commented-out forbidden methods.Several slot methods are commented out from
FORBIDDEN_SLOT_METHODSwithout clear explanation:
- Line 920:
__repr__is commented with no explanation- Line 921:
__str__has a placeholder"???"for the trait name- Lines 930-932: Descriptor methods (
__get__,__set__,__delete__) are commented out- Lines 984-992: Sequence/mapping/iterator methods (
__len__,__contains__,__getitem__,__setitem__,__delitem__,__iter__,__next__) are commented outWhile line 927-928 provides excellent documentation for why
__getattribute__,__setattr__, and__delattr__are intentionally not forbidden, the other commented-out entries lack similar justification.Please either:
- Add explanatory comments for each commented-out entry explaining why these methods are allowed to be implemented via
#[pymethod]- Uncomment entries that should be forbidden and provide the correct trait names
- Remove commented-out entries if they're not relevant to keep the code clean
Example documentation pattern
// Representable trait -// ("__repr__", "Representable"), -// ("__str__", "???"), // allow __str__ +// NOTE: __repr__ and __str__ are intentionally allowed via pymethod because [reason] +// ("__repr__", "Representable"),
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/derive-impl/src/pyclass.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/derive-impl/src/pyclass.rs
🔇 Additional comments (1)
crates/derive-impl/src/pyclass.rs (1)
1002-1014: Excellent error message for forbidden slot methods.The check logic and error message are well-designed:
- Clear identification of which method is forbidden
- Actionable guidance to use trait implementation instead
- Educational note that slot methods are exposed as
wrapper_descriptorautomatically- Correctly scoped to non-trait implementations only
This provides a good developer experience by guiding users toward the correct pattern.
Either #6485 or this one is selectable.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.