GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||
GeezerVison is showing github.com — Update operator from 3.13.5 by ShaharNaveh · Pull Request #5935 · RustPython/RustPython 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
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertIs(dunder, orig) | ||
|
|
||
| @support.requires_docstrings | ||
| def test_attrgetter_signature(self): |
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.
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:)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
Maybe it is a base class of multiple other tests
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
f80555f to
770a000
Compare
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: 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| # TODO: RUSTPYTHON fails only when running inside COperatorTestCase | ||
| if type(self).__name__ == 'COperatorTestCase': | ||
| self.skipTest("TODO: RUSTPYTHON") |
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.
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()
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
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.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
youknowone
left a comment
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.
👍
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.