Create ContentItem with a macro #5859
Conversation
WalkthroughThis change refactors six internal struct and trait implementations in the codebase by introducing a macro, which generates both the struct and its associated trait implementation. The macro is invoked for each struct, eliminating repetitive code while preserving existing logic and behavior. No public interfaces are affected. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (11)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
derive-impl/src/pyclass.rs (2)
656-670: Solid DRY refactor – consider adding#[derive(Debug)]to ease debuggingThe macro cleanly eliminates six nearly-identical struct / impl pairs.
For internal tooling like this, the structs occasionally appear in error messages or logs; havingDebug(and maybeClone,Copy) automatically derived provides immediate value at zero cost.- #[doc = $doc] - struct $name { + #[doc = $doc] + #[derive(Debug)] + struct $name { inner: ContentItemInner<AttrName>, }
673-678: Macro invocations look correct – remember to syncimpl_item_new&ALL_ALLOWED_NAMESwhen adding more itemsThe new calls are concise and self-documenting.
Future additions (e.g. a hypotheticalValidatorItem) must be registered in three places:
define_content_item!(…)invocationALL_ALLOWED_NAMESconstantimpl_item_newmatch armA short comment near the macro could help reduce the chance of drifting definitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
derive-impl/src/pyclass.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: For documentation, generate it with 'cargo doc --no-deps --all' and refer to the architecture and development guide documents for project structure and setup.
derive-impl/src/pyclass.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
derive-impl/src/pyclass.rs
Outdated
| &self.inner | ||
| } | ||
| } | ||
| define_content_item!(struct MethodItem, "#[pymethod] and #[pyclassmethod]"); |
There was a problem hiding this comment.
Just in case, is this also possible?
| define_content_item!(struct MethodItem, "#[pymethod] and #[pyclassmethod]"); | |
| define_content_item! { | |
| /// #[pymethod] and #[pyclassmethod] | |
| struct MethodItem | |
| }; |
There was a problem hiding this comment.
Haven't thought of doing that. checking
There was a problem hiding this comment.
From my tests it is possible, had to change the macro a bit to capture the meta attrs (if present), also added the ability to optionally add pub (and variants) in front of the struct
Summary by CodeRabbit