| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 4cd61388-6679-45c5-9782-254efe25d0cc 📥 CommitsReviewing files that changed from the base of the PR and between 4ababea and 07b792b. 📒 Files selected for processing (1)
📝 Walkthrough WalkthroughAdds Comparable and Hashable implementations for the PyNone singleton (including a fixed NONE_HASH); removes many Python-level dunder wrapper method signatures from slot-related traits; removes explicit repr wrappers for struct sequences; and expands tests to assert None's comparison behaviors and hash value. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/types.py dependencies:
dependent tests: (52 tests)
Legend:
|
Sorry, something went wrong.
|
As a memo, it seems 765133c commit added pymethods to Comparable again but I can't guess the reason. It may be okay if all tests pass. |
Sorry, something went wrong.
There was a problem hiding this comment.
crates/vm/src/builtins/singletons.rs (1)🤖 Prompt for all review comments with AI agents53-54: Consider adding IMMUTABLETYPE flag for consistency.
The Comparable and Hashable additions are correct. However, note that PyNotImplemented (line 122) includes flags(IMMUTABLETYPE) while PyNone does not. Both are singletons with similar semantics, so you may want to add this flag here for consistency.
Optional consistency fix🤖 Prompt for AI Agents-#[pyclass(with(Constructor, AsNumber, Comparable, Hashable, Representable))] +#[pyclass(with(Constructor, AsNumber, Comparable, Hashable, Representable), flags(IMMUTABLETYPE))] impl PyNone {}Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/singletons.rs` around lines 53 - 54, Add the IMMUTABLETYPE flag to PyNone's pyclass attribute to match PyNotImplemented: update the PyNone declaration (#[pyclass(with(Constructor, AsNumber, Comparable, Hashable, Representable))] impl PyNone {}) to include flags(IMMUTABLETYPE) so the singleton is marked immutable like PyNotImplemented.
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/vm/src/builtins/singletons.rs`:
- Around line 53-54: Add the IMMUTABLETYPE flag to PyNone's pyclass attribute to
match PyNotImplemented: update the PyNone declaration
(#[pyclass(with(Constructor, AsNumber, Comparable, Hashable, Representable))]
impl PyNone {}) to include flags(IMMUTABLETYPE) so the singleton is marked
immutable like PyNotImplemented.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f03bde30-5a15-45cc-a93f-89952ea2194b
📥 CommitsReviewing files that changed from the base of the PR and between 6a08fda and 4ababea.
📒 Files selected for processing (1)
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good, and thank you for catching legacy python magic methods.
There must be a bunch more of those unnecessary magic methods. Could you investigate more about other methods too?
Sorry, something went wrong.
Co-authored-by: Jeong, YunWon <69878+youknowone@users.noreply.github.com>
Okay, I will do in the next pull request. |
Sorry, something went wrong.
* Define only wrapper_descriptor * Implement missing slots for NoneType
* Define only wrapper_descriptor * Implement missing slots for NoneType
| Back | FazBrowse Home | New Git URL |
This pull request does two things. First, as the title, it implements missing slots (e.g., __eq__) for NoneType.
And it lets slot traits (e.g., Comparable) implement only the slot (wrapper_descriptor), not the method. This task was ongoing, and this commit may complete it (#6486).
It will remove NoneType from whats_left.
Summary by CodeRabbit
Improvements
Behavior Changes
Tests