Update operator from 3.13.5 #5935
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe error messages in the Changes
Poem
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
|
| @@ -555,6 +603,31 @@ def test_dunder_is_original(self): | |||
| if dunder: | |||
| self.assertIs(dunder, orig) | |||
|
|
|||
| @support.requires_docstrings | |||
| def test_attrgetter_signature(self): | |||
There was a problem hiding this comment.
I have tried to mark those signature tests with unittest.expectedFailure but then the test fails on "UNEXPECTED SUCCESS", very strange.
I have tried to mess with defining __text_signature__ but I'm guessing that I'm doing something wrong because it completely ignores it. I would appreciate any suggestion that you have:)
There was a problem hiding this comment.
Maybe it is a base class of multiple other tests
ShaharNaveh
force-pushed
the
update-operator
branch
from
f80555f to
770a000
Compare
![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 (1)
vm/src/stdlib/operator.rs (1)
252-260: Keepconcat()andiconcat()error strings consistent
iconcatnow emits
'{}' object can't be concatenated
butconcat(lines 162-170) still uses the unquoted version. For user-visible consistency (and parity with CPython), updateconcattoo:- return Err( - vm.new_type_error(format!("{} object can't be concatenated", a.class().name())) - ); + return Err(vm.new_type_error(format!( + "'{}' object can't be concatenated", + a.class().name() + )));
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/operator.pyis excluded by!Lib/**Lib/test/test_operator.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/stdlib/operator.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto 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
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/stdlib/operator.rs
🧠 Learnings (1)
vm/src/stdlib/operator.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
⏰ 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). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run tests under miri
- 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: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
vm/src/stdlib/operator.rs (1)
223-234: Error-message wording now matches CPython – looks goodThe switch to
"'{}' object cannot be interpreted as an integer"mirrors CPython exactly and keeps the quotes consistent with other built-ins. No functional impact, Clippy shows no issues.
Lib/test/test_operator.py
Outdated
| # TODO: RUSTPYTHON fails only when running inside COperatorTestCase | ||
| if type(self).__name__ == 'COperatorTestCase': | ||
| self.skipTest("TODO: RUSTPYTHON") |
There was a problem hiding this comment.
This way of handling makes hard to fix it when it is actually fixed.
I know we have a few tests having similar patterns. They must be also fixed.
A recommended pattern is adding another test function under COperatorTestCase.
class COperatoorTestCase(...): # existing code
# TODO: RUSTPYTHON
def test_attrgetter_signature(self):
super().test_attrgetter_signature()
There was a problem hiding this comment.
This way of handling makes hard to fix it when it is actually fixed. I know we have a few tests having similar patterns. They must be also fixed.
sure, np
Summary by CodeRabbit