Repository navigation
Preserve explicit customizations during canonical library migration - #9142
Conversation
Add bounded runtime, call, and doctest preservation plus explicit remaining-marker metadata. Support mapped legacy test-package migrations without losing ordered guards, and exclude cache directories from both migration paths while retaining canonical assets. Validation: 53 bounded host-Python tooling tests passed. Tests inspect source and temporary files without executing migrated fixtures. Existing copy-library cache protection is unchanged. No Lib or interpreter changes. Assisted-by: OpenAI Codex:model version unavailable
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 4
✨ 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 |
Keep migration orchestration and guarded package moves in cmd_migrate, and marker/doctest metadata in patch_spec. Reuse AST anchor validation and existing test modules, removing all eleven newly introduced helper, test, and data files. Require caller-selected doctest metadata instead of embedding syntax-case policy. Reject guarded legacy doctests before package writes. Preserve the prior 53 test scenarios and add seven focused metadata regressions; all 60 bounded host tooling tests passed. No Lib, workflow, or native changes. Assisted-by: OpenAI Codex:model version unavailable
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f565a87ba2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise ValueError(f"Legacy file overlaps canonical package: {name}") | ||
| legacy_files.append(legacy) | ||
| legacy_content = legacy.read_text(encoding="utf-8") | ||
| if extract_doctest_guards(legacy_content): |
There was a problem hiding this comment.
Reject guards from every legacy docstring
When a mapped legacy file contains a guarded doctest in a function or class docstring, this condition is false because extract_doctest_guards() only parses the module's first statement. Existing cases at Lib/test/test_pdb.py:1963-1968 demonstrate that RustPython guards do live in function docstrings; migrate_package() will therefore write the canonical package and unlink the legacy file while silently losing those guards, despite intending to reject guarded legacy doctests. Scan all doctest-bearing docstrings (or conservatively detect the custom directive) before writing.
Useful? React with 👍 / 👎.
| if not metadata: | ||
| return canonical |
There was a problem hiding this comment.
Handle empty doctest selection before parsing
When an old file has guarded module doctests but the new canonical revision removes its module docstring, an explicit "doctests": {} is the documented way to retire those guards. Both _examples() calls at lines 625-626 run before this branch, however, so the canonical input raises Missing module doctest and the migration cannot perform that retirement. Return the canonical content for an empty selection before requiring either docstring.
Useful? React with 👍 / 👎.
| for target, data in contents.items(): | ||
| destination = lib_dir / target | ||
| destination.parent.mkdir(parents=True, exist_ok=True) | ||
| destination.write_bytes(data) |
There was a problem hiding this comment.
Preserve source modes when writing package assets
On POSIX, when the canonical package contains an executable script or fixture, write_bytes() creates the new destination with the process-default mode (typically 0644) rather than the source mode. The repository already tracks examples such as Lib/test/test_importlib/resources/update-zips.py as 0755, so a package migration containing such an asset silently strips its executable bit, unlike patch_directory()'s copy2 path. Copy or restore the source mode after writing the cached content.
Useful? React with 👍 / 👎.
Summary
Improve canonical library migration using the existing updater architecture. The net PR changes exactly four existing files and adds no files:
cmd_migrate.py: migration orchestration, mapped legacy-package moves, shared AST-anchor validation for narrowly identified existing customizations, and directory-cache filtering.patch_spec.py: decorator metadata ordering/subset validation and exact-input doctest guard extraction/application.test_migrate.pyandtest_patch_spec.py: regression coverage for those paths.The initial helper modules, separate test modules, and syntax-case JSON have been removed. There are no
Lib/, workflow, or native interpreter changes, and the already-landed copy-library cache fix is preserved.Behavior
doctestsmetadata keyed by exact existing guarded input. There is no permanent list of syntax cases in the updater. Canonical expected output is retained. Migrating guarded doctests without an explicit selection fails before writing; an explicit empty selection retires their guards.__pycache__and legitimate bytecode fixtures.Legacy-package semantic validation occurs before writes. This does not promise filesystem-transaction rollback if an I/O operation fails while writing or removing a legacy file.
Validation
Tests inspect source and temporary files without executing migrated fixture modules. No interpreter build or native behavior test was run for this tooling-only change.