Remake update_lib#6796
Conversation
📝 WalkthroughWalkthroughThe PR replaces the monolithic Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as update_lib<br/>(quick)
participant Migrate as migrate.py
participant AutoMark as auto_mark.py
participant TestRunner as run_test()
participant FileSystem as File System
User->>CLI: quick [path] --no-migrate=False
CLI->>Migrate: patch_file/patch_directory
Migrate->>FileSystem: Extract patches from Lib/test/<file>.py
Migrate->>FileSystem: Read cpython/Lib/test/<file>.py
Migrate->>FileSystem: Apply patches & write to Lib/test/<file>.py
Migrate-->>CLI: Patching complete
CLI->>AutoMark: auto_mark_file/auto_mark_directory
AutoMark->>TestRunner: run_test(test_name)
TestRunner->>FileSystem: Execute RustPython tests
TestRunner-->>AutoMark: TestResult{failures, unexpected_successes}
AutoMark->>AutoMark: Parse AST to extract test methods
AutoMark->>AutoMark: Build inheritance info & resolve method defs
AutoMark->>AutoMark: build_patches for failures
AutoMark->>FileSystem: Remove `@expectedFailure` from passing tests
AutoMark->>FileSystem: Apply patches to mark new failures
AutoMark-->>CLI: Auto-marking complete (delta counts)
CLI->>FileSystem: Optional: git commit with version tag
CLI-->>User: Update workflow finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin lib_updater |
Sorry, something went wrong.
03ee58f to
1813c7d
Compare
January 19, 2026 12:11
|
I knew I could push directly to this branch, but I was cautious so I sent a PR: youknowone#10 |
Sorry, something went wrong.
212d54f to
fd0bc1b
Compare
January 19, 2026 15:15
fd0bc1b to
b295586
Compare
January 20, 2026 02:33
|
I ran this for dataclasses, inspect, test_types and working all good. Let's fix other bugs one by one later |
Sorry, something went wrong.
f1fd1e9
into
RustPython:main
Jan 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/update_lib/patch_spec.py (1)
295-302: Handle files without imports to avoid assertion crashes.
If a test file has no imports,_find_import_insert_lineasserts andapply_patchesfails when addingimport unittest. Add a safe fallback (e.g., after module docstring or at top).🐛 Proposed fix
- assert last_import_line is not None - return last_import_line + if last_import_line is not None: + return last_import_line + # Fallback: insert after module docstring if present, else at top + if ( + tree.body + and isinstance(tree.body[0], ast.Expr) + and isinstance(tree.body[0].value, ast.Constant) + and isinstance(tree.body[0].value.value, str) + ): + return tree.body[0].end_lineno or tree.body[0].lineno + return 0
🤖 Fix all issues with AI agents
In @.claude/commands/upgrade-pylib.md:
- Around line 15-20: The markdown violates MD007 because the unordered list
items under "Describe:" are indented by two spaces; fix by removing the
two-space indentation so the "-" list markers start at column 0 (e.g., adjust
the lines containing "- Library name" and "- The full command executed..." to
have no leading spaces), and scan the rest of .claude/commands/upgrade-pylib.md
for any other unordered list items with extra leading spaces and normalize them
to 0 indentation to satisfy the markdownlint config.
In `@scripts/update_lib/auto_mark.py`:
- Around line 227-244: The helper _is_super_call_only currently only verifies
that the body returns a super() call but doesn't ensure the delegated attribute
matches the current function, which can prune methods like def foo(self): return
super().bar(); update _is_super_call_only to verify that the ast.Attribute on
the Call has attr equal to func_node.name (i.e., ensure call.func.attr ==
func_node.name) before returning True; keep the existing checks for the AST
shapes and optionally ensure arguments/keywords match if you want stronger
equivalence, but at minimum add the attribute-name equality guard in
_is_super_call_only.
In `@scripts/update_lib/copy_lib.py`:
- Around line 45-54: For directory copies, ensure the destination parent exists
before calling shutil.copytree: before calling shutil.copytree(src_path,
lib_path) create lib_path.parent (using mkdir with parents=True, exist_ok=True)
just like the file branch does; update the block that checks src_path.is_dir()
to call lib_path.parent.mkdir(parents=True, exist_ok=True) (and keep the verbose
print) so copytree won’t raise FileNotFoundError.
In `@scripts/update_lib/path.py`:
- Around line 42-78: The lib_to_test_path function mishandles package
__init__.py names (producing test___init__); update both branches (the "/Lib/"
and "Lib/" branches in lib_to_test_path) to detect when src_path.name ==
"__init__.py" (or lib_path.stem == "__init__") and in that case set lib_name to
the parent directory's name (e.g., lib_path.parent.name or src_path.parent.name)
instead of "__init__"; keep the existing directory-first then file-fallback
logic and continue returning the constructed pathlib.Path values as before.
In `@scripts/update_lib/tests/test_migrate.py`:
- Around line 1-196: The new tests in TestPatchSingleContent, TestPatchFile, and
TestPatchDirectory add general test logic and assertions which violates the
test-file modification policy; to fix, move these checks out of a "*test*.py"
file into a non-test helper/module (e.g., create a new module like
update_lib/tests_helpers.py) or replace the changes so that the only
modifications inside the test files are adding/removing
`@unittest.expectedFailure` with TODO comments (references: patch_single_content,
patch_file, patch_directory, and the test classes
TestPatchSingleContent/TestPatchFile/TestPatchDirectory), then update the
import/usage accordingly or request a policy exception.
🧹 Nitpick comments (6)
scripts/update_lib/patches.py (1)
24-30: Use explicit UTF‑8 encoding when writing output.
Locale defaults can vary; explicit UTF‑8 keeps patch output deterministic across environments.♻️ Proposed tweak
- with open(dest, "w") as fd: + with open(dest, "w", encoding="utf-8") as fd:scripts/update_lib/path.py (1)
88-99: Consider adding type hint for consistency.The function works correctly for the documented use cases. Consider adding a type hint for the parameter to match the style of other functions in this module:
Suggested improvement
-def test_name_from_path(test_path: pathlib.Path) -> str: +def test_name_from_path(test_path: pathlib.Path | str) -> str:scripts/update_lib/quick.py (4)
43-64: Consider specifying encoding for file reads.Lines 59 and 63 use
read_text()without explicit encoding. While Python 3 defaults to UTF-8 on most systems, explicitly specifyingencoding="utf-8"improves cross-platform consistency and makes the intent clear.Suggested improvement
if lib_path.is_file(): - return extract_test_methods(lib_path.read_text()) + return extract_test_methods(lib_path.read_text(encoding="utf-8")) else: result = {} for lib_file in get_test_files(lib_path): - result[lib_file.resolve()] = extract_test_methods(lib_file.read_text()) + result[lib_file.resolve()] = extract_test_methods(lib_file.read_text(encoding="utf-8")) return result
166-225: Consider using a placeholder email address.The author string
"CPython Developers <>"uses an empty email which may cause issues with certain git configurations or CI systems that validate commit metadata. Consider using a no-reply or placeholder email.Suggested improvement
subprocess.run( [ "git", "commit", "--author", - "CPython Developers <>", + "CPython Developers <noreply@python.org>", "-m", message, ], check=True, )
228-260: Minor: Windows path separator not handled in shortcut check.Line 237 only checks for
"/"but not"\\", which could cause issues on Windows where paths might use backslashes. Consider normalizing first:Suggested improvement
def _expand_shortcut(path: pathlib.Path) -> pathlib.Path: # Only expand if it's a simple name (no path separators) and doesn't exist - if "/" in str(path) or path.exists(): + path_str = str(path).replace("\\", "/") + if "/" in path_str or path.exists(): return path
263-365: Consider handlingsubprocess.CalledProcessErrorfor git operations.The
main()function catchesValueErrorandFileNotFoundErrorbut notsubprocess.CalledProcessError. If git commands fail (e.g.,get_cpython_versionorgit_commit), the error will result in an uncaught exception with a full traceback instead of a clean error message.Suggested improvement
+ except subprocess.CalledProcessError as e: + print(f"Git command failed: {e}", file=sys.stderr) + return 1 except ValueError as e: print(f"Error: {e}", file=sys.stderr) return 1Also add at the top of the file:
import argparse import pathlib +import subprocess import sys
Sorry, something went wrong.
cc @moreal
I am going to rewrite this tools more organized library update toolkit, and remain rooms for further automation.
scripts/update_libwill be main entrance.lib_updater
->
scripts/update_lib patches(--from --to)->
scripts/update_lib migrate(--quick-upgrade)auto_mark_test
->
scripts/update_lib auto_markAlso
scripts/update_lib quickis added to upgrade a whole package at once, copy the library, remove unexected sucesss new add expectedFailure for newly added failing tests but not about regression.The format also will be aligned to ruff format and PEP8 to fix #6701.
Keeping draft to test for a few more libraries. Please feedback.
Summary by CodeRabbit
Documentation
update_libworkflow with improved step-by-step guidanceRefactor
auto_mark_test.pytoolupdate_libutility offering enhanced test marker preservation and improved failure detection workflow✏️ Tip: You can customize this high-level summary in your review settings.