Complement upgrade-pylib Claude Code command - #6742
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughAdds a detailed multi-step test-upgrade workflow document and updates scripts/lib_updater.py to detect and insert a missing import unittest into the import block when applying modifications that require it. 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)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin enhance-upgrade-pylib |
Sorry, something went wrong.
| 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
Sorry, something went wrong.
There was a problem hiding this comment.
You're right! 😓 I'll fix it now...
Sorry, something went wrong.
There was a problem hiding this comment.
we can maybe adjust the decorator to __import__('unittest').expectedFailure and not needing this patch at all?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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:/
Sorry, something went wrong.
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
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agentsIn `@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).
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 746e71a and 708f227.
📒 Files selected for processing (2)- .claude/commands/upgrade-pylib.md
- scripts/lib_updater.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
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
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
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
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.md
- scripts/lib_updater.py
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
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
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
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
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
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
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
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
- 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
.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.
Sorry, something went wrong.
|
@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! |
Sorry, something went wrong.
| 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
Sorry, something went wrong.
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?
Sorry, something went wrong.
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
Sorry, something went wrong.
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=[])
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, you're right 🤦
let's go with that approach
Sorry, something went wrong.
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!
Sorry, something went wrong.
ShaharNaveh
left a comment
There was a problem hiding this comment.
LGTM!
@youknowone can we merge?
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
@moreal @ShaharNaveh Thank you so much!
Sorry, something went wrong.
The command was improved based on the process of updating the json module using the /update-pylib command. Additionally, the script was modified to add import unittest if 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.