Complement upgrade-pylib Claude Code command #6742
Conversation
📝 WalkthroughWalkthroughAdds a detailed multi-step test-upgrade workflow document and updates Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin enhance-upgrade-pylib |
moreal
force-pushed
the
enhance-upgrade-pylib
branch
from
fab8624 to
708f227
Compare
scripts/lib_updater.py
Outdated
| for alias in node.names: | ||
| if alias.name == UT: | ||
| return True | ||
| elif isinstance(node, ast.ImportFrom): |
There was a problem hiding this comment.
I think we should remove this elif since we want to bring unittest into scope
There was a problem hiding this comment.
You're right! 😓 I'll fix it now...
There was a problem hiding this comment.
we can maybe adjust the decorator to __import__('unittest').expectedFailure and not needing this patch at all?
There was a problem hiding this comment.
That would certainly allow us to fully resolve the question of where to place the imports, but the code does feel a bit awkward to read, and I also wonder whether calling __import__ every time—while not significant—might slightly slow down the tests. I don’t have strong convictions or a firm opinion on this.
I’ll follow the maintainers’ decision.
There was a problem hiding this comment.
In CPython this will have very minimal performance cost due to modules being cached. I ain't sure about RustPython:/
There was a problem hiding this comment.
Yes, that’s right. I think it’s probably well optimized. It just feels a bit awkward to look at code like the example below.
@__import__('unittest').expectedFailure
def test_xxx():
# SomethingFunctionally, there’s no major issue, so I’m mainly interested in hearing the maintainers’ (i.e., the people primarily responsible for maintenance) opinions. Would you be able to share your thoughts on this thread or on this PR? @youknowone
There was a problem hiding this comment.
We can maybe check if there's a import unittest node in the AST and switch between @unittest.X or __import__('unittest').X, as I feel like most tests do import unittest
There was a problem hiding this comment.
Well, while that will work perfectly, we still need to keep them human friendly.
Most of people with python-background are familiar with @unittest.X. So let's stick on it.
Performance doesn't matter.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/lib_updater.py`:
- Around line 273-283: has_unittest_import currently treats both "from unittest
import ..." and aliased imports as if the name unittest is available, which is
incorrect for decorators using `@unittest`.*; update has_unittest_import to only
return True when there is an ast.Import whose alias.name equals UT and
alias.asname is None (i.e., a plain "import unittest"), and do not treat
ast.ImportFrom (from unittest import ...) or imports with asname (import
unittest as ut) as satisfying the requirement; reference the has_unittest_import
function and the UT symbol when making this change.
- Around line 286-309: The current logic inserts "import unittest" at line 0
when no imports exist, which can place it before a module docstring or
encoding/shebang; update the insertion logic so it picks a safe line after any
shebang/encoding comments and the module docstring: either enhance
find_import_insert_line to detect and return the end line of the module
docstring (use ast.get_docstring(tree, clean=False) or inspect tree.body[0] if
it's an ast.Expr/ast.Constant string node) and use that end_lineno if > 0, and
also scan the top of the original file lines for a shebang (#!) and PEP-263
encoding comment and advance the insertion line past them; apply this change
where apply_patches calls find_import_insert_line (and keep the check using
modifications, has_unittest_import, and iter_patch_lines intact).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.claude/commands/upgrade-pylib.mdscripts/lib_updater.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code
Files:
scripts/lib_updater.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/test/**/*.py : Use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment when marking tests in Lib/ that cannot run
📚 Learning: 2026-01-14T14:52:10.778Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the Lib/ directory; these files should be edited very conservatively and modifications should be minimal and only to work around RustPython limitations
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2026-01-14T14:52:10.778Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to Lib/ directory files
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Applied to files:
.claude/commands/upgrade-pylib.mdscripts/lib_updater.py
📚 Learning: 2026-01-14T14:52:10.778Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to Lib/test/**/*.py : Use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment when marking tests in Lib/ that cannot run
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2026-01-14T14:52:10.779Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Do not edit the Lib/ directory directly except for copying files from CPython to work around RustPython limitations
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2026-01-14T14:52:10.779Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: When comparing behavior with CPython, use `python` command to explicitly run CPython and `cargo run -- script.py` to run RustPython
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2026-01-14T14:52:10.779Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Use `cargo run -- script.py` instead of `python script.py` when testing Python code with RustPython
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2026-01-14T14:52:10.779Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: Run a full clean build when modifying bytecode instructions using: `rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r`
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2026-01-14T14:52:10.779Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.779Z
Learning: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Applied to files:
.claude/commands/upgrade-pylib.md
📚 Learning: 2025-09-07T05:38:31.690Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:198-202
Timestamp: 2025-09-07T05:38:31.690Z
Learning: In scripts/lib_updater.py, the iter_patches function intentionally does not handle SyntaxError from ast.parse(contents). The author confirmed this behavior is fine and intended - the tool should fail fast on unparseable files rather than silently skip processing.
Applied to files:
scripts/lib_updater.py
📚 Learning: 2026-01-14T14:52:10.778Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-14T14:52:10.778Z
Learning: Applies to **/test*.py : Only remove `unittest.expectedFailure` decorators and upper TODO comments from tests when tests actually pass, or add these decorators when tests cannot be fixed
Applied to files:
scripts/lib_updater.py
⏰ 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). (8)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
.claude/commands/upgrade-pylib.md (1)
18-107: Documentation updates are clear and actionable.The expanded workflow and examples align well with the updated tooling and should reduce operator confusion.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
moreal
force-pushed
the
enhance-upgrade-pylib
branch
from
708f227 to
6679e0e
Compare
moreal
force-pushed
the
enhance-upgrade-pylib
branch
from
6679e0e to
95f822d
Compare
|
@ShaharNaveh As the major author of lib_updater, please be a major reviewer of this PR. I will merge it once you approve it. Thanks! |
scripts/lib_updater.py
Outdated
| if lines: | ||
| insert_line = 0 | ||
| for i, line in enumerate(lines[:2]): # Only check first two lines per PEP 263 | ||
| if line.startswith("#!") or re.match( |
There was a problem hiding this comment.
Maybe this entire function is a bit of an overkill? inserting the import before the first:
- class
- function
- constant
do the trick? we don't have lots of files that don't import unittest
There was a problem hiding this comment.
Come to think of it, Python test.test_* modules are for testing something, so they inherently have imports. I think the function's complexity is from implementing the suggestion of the below comment, so I'll revert it to first implementation:
Like following lines:
def find_import_insert_line(tree: ast.Module) -> int:
"""Find the line number after the last import statement."""
last_import_line = None
for node in tree.body:
if isinstance(node, (ast.Import, ast.ImportFrom)):
last_import_line = node.end_lineno or node.lineno
assert last_import_line is not None # expect passing because test module might have `import x` to test `x`
return last_import_lineHow do you think about it?
There was a problem hiding this comment.
This has an issue in the case of an import inside a test.
import posix
def test_posix_import():
import nt
...I'd put a condition to check that it it's indent is 0.
Or, insert it after the first import. It won't be pretty but I think it's putting a lot of effort to solve an uncommon problem
There was a problem hiding this comment.
I don't think such a case would be an issue because it iterates ast.Module.body not recursively like visitor pattern. So, as long as the import is at the top like the usual pattern, it should be fine.
>>> import ast
>>> code = """import posix
...
... def test_posix_import():
... import nt
... ..."""
>>> ast.parse(code)
Module(body=[Import(names=[alias(name='posix', asname=None)]), FunctionDef(name='test_posix_import', args=arguments(posonlyargs=[], args=[], vararg=None, kwonlyargs=[], kw_defaults=[], kwarg=None, defaults=[]), body=[Import(names=[alias(...)]), Expr(value=Constant(...))], decorator_list=[], returns=None, type_comment=None, type_params=[])], type_ignores=[])
>>> tree = ast.parse(code)
>>> for node in tree.body: print(node)
...
Import(names=[alias(name='posix', asname=None)])
FunctionDef(name='test_posix_import', args=arguments(posonlyargs=[], args=[], vararg=None, kwonlyargs=[], kw_defaults=[], kwarg=None, defaults=[]), body=[Import(names=[alias(name='nt', asname=None)]), Expr(value=Constant(value=Ellipsis, kind=None))], decorator_list=[], returns=None, type_comment=None, type_params=[])There was a problem hiding this comment.
Oh, you're right 🤦
let's go with that approach
There was a problem hiding this comment.
@ShaharNaveh I just reverted the previous commit (at 5b54ec9) and added assert ... line for assumption (at 80dfec2). Please take a look!
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
LGTM!
@youknowone can we merge?
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
@moreal @ShaharNaveh Thank you so much!
3 participants
The command was improved based on the process of updating the json module using the
/update-pylibcommand. Additionally, the script was modified to addimport unittestif it is missing.I worked on #6743 with this pull request's changes.
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.