-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Deprecate ::new_ref #6046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate ::new_ref #6046
Conversation
WalkthroughThis change deprecates the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BuiltinType
participant Context
Caller->>BuiltinType: from(data)
BuiltinType->>BuiltinType: construct instance
Caller->>BuiltinType: into_ref(ctx)
BuiltinType->>Context: create PyRef
Context-->>Caller: PyRef<BuiltinType>
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (15)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (8)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (2)vm/src/stdlib/marshal.rs (1)Learnt from: CR vm/src/macros.rs (1)Learnt from: CR 🔇 Additional comments (5)
✨ 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. 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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
vm/src/builtins/bytearray.rs
(1 hunks)vm/src/builtins/bytes.rs
(1 hunks)vm/src/builtins/classmethod.rs
(1 hunks)vm/src/builtins/complex.rs
(1 hunks)vm/src/builtins/dict.rs
(1 hunks)vm/src/builtins/function.rs
(1 hunks)vm/src/builtins/list.rs
(1 hunks)vm/src/builtins/staticmethod.rs
(1 hunks)vm/src/builtins/str.rs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs
: Follow the default rustfmt code style (cargo fmt
to format)
Always run clippy to lint code (cargo clippy
) before completing tasks. Fix any warnings or lints that are introduced by your 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:
vm/src/builtins/staticmethod.rs
vm/src/builtins/dict.rs
vm/src/builtins/bytes.rs
vm/src/builtins/bytearray.rs
vm/src/builtins/complex.rs
vm/src/builtins/str.rs
vm/src/builtins/list.rs
vm/src/builtins/function.rs
vm/src/builtins/classmethod.rs
🔇 Additional comments (11)
vm/src/builtins/dict.rs (1)
54-57
: LGTM! Clean deprecation and simplification.The deprecation note correctly guides users to the new pattern, and the implementation has been properly simplified to use
Self::default().into_ref(ctx)
directly. This is consistent with the broader refactoring effort.vm/src/builtins/bytes.rs (1)
102-105
: LGTM! Consistent with the refactoring pattern.The deprecation note correctly guides users to the new
PyBytes::from(...).into_ref()
pattern, and the implementation has been properly simplified to use exactly that approach. This change is consistent with the broader refactoring effort across built-in types.vm/src/builtins/bytearray.rs (1)
76-79
: LGTM! Follows the established refactoring pattern.The deprecation note correctly guides users to the new
PyByteArray::from(...).into_ref()
pattern, and the implementation has been properly simplified to use exactly that approach. This change is consistent with similar refactoring across other built-in types in this PR.vm/src/builtins/str.rs (1)
421-425
: LGTM: Well-implemented deprecation patternThe deprecation is properly implemented with:
- Clear guidance in the deprecation note directing users to use
PyStr::from(...).into_ref()
instead- Simplified implementation that delegates to
into_ref(ctx)
while maintaining the same functionality- Maintains backward compatibility during the transition period
This aligns well with the PR's objective to standardize instance creation patterns across built-in types.
vm/src/builtins/staticmethod.rs (2)
64-68
: LGTM: Clean constructor implementationThe new
new
method provides a clean, idiomatic constructor that:
- Takes the required
PyObjectRef
parameter- Properly wraps it in
PyMutex
as expected by the struct- Follows standard Rust constructor patterns
This serves as the foundation for the deprecation pattern being implemented across the codebase.
69-72
: LGTM: Consistent deprecation implementationThe deprecation follows the established pattern:
- Clear deprecation note directing users to
PyStaticMethod::new(...).into_ref()
- Simplified implementation that delegates to the new constructor method
- Maintains backward compatibility during the transition
This is consistent with the deprecation pattern being applied across all built-in types in this PR.
vm/src/builtins/function.rs (1)
776-779
: LGTM: Consistent deprecation pattern appliedThe deprecation of
PyBoundMethod::new_ref
properly follows the established pattern:
- Clear deprecation note with specific guidance to use
Self::new(object, function).into_ref(ctx)
- Simplified implementation that leverages the existing
Self::new
constructor- Maintains backward compatibility during the transition period
This completes the consistent application of the deprecation pattern across built-in types as outlined in the PR objectives.
vm/src/builtins/classmethod.rs (1)
114-117
: LGTM! Clean deprecation implementation.The deprecation follows Rust best practices with a clear migration path. The implementation correctly uses the idiomatic
Self::from(callable).into_ref(ctx)
pattern, which aligns with the PR objective to standardize constructor methods across builtin types.vm/src/builtins/complex.rs (3)
231-234
: LGTM! Consistent deprecation implementation.The deprecation follows the same pattern as other builtin types in this PR, providing a clear migration path with the idiomatic
Self::from(value).into_ref(ctx)
pattern.
236-238
: Good optimization with const fn.Making
to_complex64
aconst fn
is a nice optimization that allows compile-time evaluation when possible, while maintaining the same functionality.
244-254
: Good refactoring with helper method.The
number_op
helper method consolidates the common pattern of converting twoPyObject
references toComplex64
values and applying a binary operation. This reduces code duplication and improves maintainability.
ce73615
to
0a05a91
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Documentation