scripts/update_lib migrate to preserve patches on classes#8057
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughPatch spec module refactored to centralize extraction via AST visitor pattern and application via shared helper. Decorator parsing moved into ChangesPatch extraction and application refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/update_lib/patch_spec.py`:
- Around line 145-149: The code reads prev_line = lines[dec_lineno - 2] without
checking for file start, which makes lines[-1] get used when a decorator is on
the first line; change the logic around dec_lineno (from node.lineno) so you
only index dec_lineno - 2 when dec_lineno > 1, otherwise set prev_line to an
empty string or None; update any downstream checks that read prev_line (in the
same function handling decorator parsing) to handle the empty/None value to
avoid treating EOF/comments as decorator metadata.
- Around line 141-143: When extracting the decorator condition in patch_spec.py
(inside the block guarded by ut_method.has_cond()), avoid indexing node.args
directly; instead if node.args is non-empty keep using node.args[0], otherwise
search node.keywords for the keyword named "condition" (i.e. iterate
node.keywords and match kw.arg == "condition") and set cond =
ast.unparse(kw.value) if found; if neither exists set cond = None or skip
extraction so the code using cond won't raise IndexError. Reference:
ut_method.has_cond(), cond variable, and node.args / node.keywords.
In `@scripts/update_lib/tests/test_patch_spec.py`:
- Around line 348-373: The new test method test_round_trip_with_patches_on_class
violates the repo policy against modifying test files; remove this added test
(the function test_round_trip_with_patches_on_class and its assertions
referencing extract_patches, apply_patches, and COMMENT) from the patch so no
new test logic or assertions are introduced in *test*.py files, or if needed,
move equivalent validation into an allowed non-test location (e.g., a new
integration/example script) and keep the test file unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4a9b38eb-6436-4447-a7cd-65fb4c870c64
📒 Files selected for processing (2)
scripts/update_lib/patch_spec.pyscripts/update_lib/tests/test_patch_spec.py
Comment on lines
+141
to
+143
| if ut_method.has_cond(): | ||
| cond = ast.unparse(node.args[0]) | ||
| else: |
Contributor
There was a problem hiding this comment.
Handle keyword-based conditional decorators without crashing.
Line 142 assumes a positional condition (node.args[0]). Valid forms like @unittest.skipIf(condition=..., reason=...) make node.args empty and raise IndexError during extraction.
Proposed fix
- if ut_method.has_cond():
- cond = ast.unparse(node.args[0])
+ if ut_method.has_cond():
+ if not isinstance(node, ast.Call):
+ return
+ if node.args:
+ cond_node = node.args[0]
+ else:
+ cond_node = next(
+ (kw.value for kw in node.keywords if kw.arg in {"condition", "cond"}),
+ None,
+ )
+ if cond_node is None:
+ return
+ cond = ast.unparse(cond_node)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/update_lib/patch_spec.py` around lines 141 - 143, When extracting the
decorator condition in patch_spec.py (inside the block guarded by
ut_method.has_cond()), avoid indexing node.args directly; instead if node.args
is non-empty keep using node.args[0], otherwise search node.keywords for the
keyword named "condition" (i.e. iterate node.keywords and match kw.arg ==
"condition") and set cond = ast.unparse(kw.value) if found; if neither exists
set cond = None or skip extraction so the code using cond won't raise
IndexError. Reference: ut_method.has_cond(), cond variable, and node.args /
node.keywords.
Comment on lines
+145
to
+149
| dec_lineno = node.lineno | ||
|
|
||
| curr_line = lines[dec_lineno - 1] | ||
| prev_line = lines[dec_lineno - 2] | ||
|
|
Contributor
There was a problem hiding this comment.
Guard previous-line lookup at file start.
Line 148 reads lines[dec_lineno - 2] unconditionally. If the decorator is on the first line, this becomes lines[-1] and can incorrectly treat EOF comments as decorator metadata.
Proposed fix
- prev_line = lines[dec_lineno - 2]
+ prev_line = lines[dec_lineno - 2] if dec_lineno > 1 else ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/update_lib/patch_spec.py` around lines 145 - 149, The code reads
prev_line = lines[dec_lineno - 2] without checking for file start, which makes
lines[-1] get used when a decorator is on the first line; change the logic
around dec_lineno (from node.lineno) so you only index dec_lineno - 2 when
dec_lineno > 1, otherwise set prev_line to an empty string or None; update any
downstream checks that read prev_line (in the same function handling decorator
parsing) to handle the empty/None value to avoid treating EOF/comments as
decorator metadata.
Comment on lines
+348
to
+373
| def test_round_trip_with_patches_on_class(self): | ||
| """Test that extracted patches can be re-applied.""" | ||
| original = f"""import unittest | ||
|
|
||
| @unittest.skipIf(a == b, "{COMMENT}") | ||
| @unittest.expectedFailure # {COMMENT} | ||
| class TestFoo(unittest.TestCase): | ||
| ... | ||
| """ | ||
| # Extract patches | ||
| patches = extract_patches(original) | ||
|
|
||
| # Apply to clean code | ||
| clean = """import unittest | ||
|
|
||
| class TestFoo(unittest.TestCase): | ||
| def test_one(self): | ||
| pass | ||
| """ | ||
| result = apply_patches(clean, patches) | ||
|
|
||
| # Should have the decorator | ||
| self.assertIn("@unittest.expectedFailure", result) | ||
| self.assertIn("@unittest.skipIf", result) | ||
| self.assertIn(COMMENT, result) | ||
|
|
Contributor
There was a problem hiding this comment.
This test addition violates the repository’s test-file change policy.
Lines 348-373 introduce new test logic/data/assertions in a *test*.py file, which is outside the allowed modification scope in this repo policy.
As per coding guidelines, **/*test*.py says: “NEVER modify test assertions, test logic, or test data” and limits acceptable edits to expectedFailure/TODO handling. Based on learnings, AGENTS guidance repeats the same restriction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/update_lib/tests/test_patch_spec.py` around lines 348 - 373, The new
test method test_round_trip_with_patches_on_class violates the repo policy
against modifying test files; remove this added test (the function
test_round_trip_with_patches_on_class and its assertions referencing
extract_patches, apply_patches, and COMMENT) from the patch so no new test logic
or assertions are introduced in *test*.py files, or if needed, move equivalent
validation into an allowed non-test location (e.g., a new integration/example
script) and keep the test file unchanged.
Sources: Coding guidelines, Learnings
youknowone
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Previously, patches on classes itself were drop when migrating patches.
Summary by CodeRabbit
Refactor
Tests