GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Use `ast.unparse` for generating patches with `lib_updater.py` by ShaharNaveh · Pull Request #6142 · RustPython/RustPython Conversation
|
WalkthroughRewrites patch generation in scripts/lib_updater.py from string-based formatting to AST-driven decorator construction, adds UtMethod.has_args, refactors reason extraction logic to AST/surrounding-source paths, introduces new constants, updates JSON load/show handling, and standardizes indentation and unittest aliasing. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer/CLI
participant LU as LibUpdater
participant AST as Python AST
participant SF as Source File
participant JSON as Patches JSON
rect rgb(235, 245, 255)
note right of LU: Phase 1/2 Patch Emission (AST-driven)
Dev->>LU: run update
LU->>AST: build decorator nodes (PatchSpec.as_ast_node)
AST-->>LU: ast.Attribute / ast.Call
LU->>LU: render as_decorator() and indent with DEFAULT_INDENT
LU->>SF: write decorators and wrappers
end
rect rgb(245, 235, 255)
note right of LU: Patch Extraction (reason/cond)
LU->>AST: parse source for UT methods
alt UT method has_args()
LU->>AST: walk dec_node for COMMENT string and optional cond
AST-->>LU: reason, cond
else No args
LU->>SF: scan surrounding lines for COMMENT
SF-->>LU: reason line
end
LU->>LU: normalize reason via removeprefix(COMMENT)
end
rect rgb(235, 255, 240)
note right of LU: JSON I/O
LU->>JSON: load patches
JSON-->>LU: dicts
LU->>LU: convert ut_method -> UtMethod via _replace
LU-->>Dev: show-patches via spec._asdict()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews: pre_merge_checks: custom_checks: - name: "Undocumented Breaking Changes" mode: "warning" instructions: | Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
||
| # TODO: RUSTPYTHON; called `Result::unwrap()` on an `Err` value: StackUnderflow | ||
| ''' | ||
| @unittest.expectedFailure # TODO: RUSTPYTHON; TypeError: class pattern defines no positional sub-patterns (__match_args__ missing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test now no longer breaks file
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
|
|
||
| # TODO: RUSTPYTHON | ||
| @unittest.skip(reason="RUSTPYTHON hangs") | ||
| @unittest.skip("TODO: RUSTPYTHON; hangs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was added manually
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/lib_updater.py (2)
149-149: Broaden detection ofunittestalias (optional).Matching only
attr_node.value.id == "unittest"missesimport unittest as ut. Consider collecting aliases from imports and/or allow a small alias set.Example minimal tweak (constant-only):
- or getattr(attr_node.value, "id", None) != UT + or getattr(attr_node.value, "id", None) not in {UT, "ut", "UT"}Or, for correctness, scan imports to discover the actual alias and check against that alias. I can draft that if you want.
328-331: Slightly simplify JSON→PatchSpec load path (optional).Avoid constructing then
_replace; also tolerate missing keys.Apply this diff:
- test_name: [ - PatchSpec(**spec)._replace(ut_method=UtMethod(spec["ut_method"])) - for spec in specs - ] + test_name: [ + PatchSpec( + ut_method=UtMethod(spec["ut_method"]), + cond=spec.get("cond"), + reason=spec.get("reason", ""), + ) + for spec in specs + ]
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
Lib/test/test_ast/test_ast.pyis excluded by!Lib/**Lib/test/test_csv.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_os.pyis excluded by!Lib/**Lib/test/test_str.pyis excluded by!Lib/**Lib/test/test_xml_etree.pyis excluded by!Lib/**
📒 Files selected for processing (1)
scripts/lib_updater.py(8 hunks)
🧰 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 (1)
📓 Common learnings
Learnt from: ShaharNaveh
PR: RustPython/RustPython#6089
File: scripts/lib_updater.py:198-202
Timestamp: 2025-09-07T05:38:31.644Z
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.
🪛 Flake8 (7.2.0)
scripts/lib_updater.py
[error] 41-41: SyntaxError: invalid syntax
(E999)
⏰ 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). (11)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
🔇 Additional comments (2)
scripts/lib_updater.py (2)
243-245: Decorator emission and phase-2 wrapper formatting look good.Switch to
textwrap.indentand centralizedDEFAULT_INDENTimproves consistency and readability.Also applies to: 253-260
35-35: Nice cleanup.Importing
textwrap, introducingDEFAULT_INDENT, centralizingUT, and adding_reason/has_args/as_decoratorstreamline formatting and reduce stringly-typed code.Also applies to: 43-46, 57-59, 87-90, 106-113
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| if typing.TYPE_CHECKING: | ||
| from collections.abc import Iterator | ||
|
|
||
| type Patches = dict[str, dict[str, list["PatchSpec"]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: PEP 695 type alias breaks under Python ≤3.11 (E999).
Flake8 E999 points here. Replace the type statement with a TypeAlias so the script runs on 3.11 (CI baseline).
Apply this diff:
-type Patches = dict[str, dict[str, list["PatchSpec"]]] +Patches: typing.TypeAlias = dict[str, dict[str, list["PatchSpec"]]]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Patches = dict[str, dict[str, list["PatchSpec"]]] | |
| Patches: typing.TypeAlias = dict[str, dict[str, list["PatchSpec"]]] |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 41-41: SyntaxError: invalid syntax
(E999)
🤖 Prompt for AI Agents
In scripts/lib_updater.py around line 41, the use of the PEP 695 "type" alias
causes Flake8 E999 on Python ≤3.11; replace it with typing.TypeAlias: add "from
typing import TypeAlias" to the imports and change the declaration to use the
TypeAlias annotation, e.g. "Patches: TypeAlias = dict[str, dict[str,
list['PatchSpec']]]", preserving the inner forward reference string for
PatchSpec.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| @property | ||
| def _attr_node(self) -> ast.Attribute: | ||
| return ast.Attribute(value=ast.Name(id=UT), attr=self.ut_method) | ||
|
|
||
| def as_ast_node(self) -> ast.Attribute | ast.Call: | ||
| if not self.ut_method.has_args(): | ||
| return self._attr_node | ||
|
|
||
| args = [] | ||
| if self.cond: | ||
| args.append(ast.parse(self.cond).body[0].value) | ||
| args.append(ast.Constant(value=self._reason)) | ||
|
|
||
| return ast.Call(func=self._attr_node, args=args, keywords=[]) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required: add ctx=ast.Load() to constructed AST nodes to make ast.unparse reliable.
ast.Attribute and ast.Name require ctx; missing ctx can cause unparse failures or incorrect trees.
Apply this diff:
- def _attr_node(self) -> ast.Attribute: - return ast.Attribute(value=ast.Name(id=UT), attr=self.ut_method) + def _attr_node(self) -> ast.Attribute: + return ast.Attribute( + value=ast.Name(id=UT, ctx=ast.Load()), + attr=str(self.ut_method), + ctx=ast.Load(), + ) - def as_ast_node(self) -> ast.Attribute | ast.Call: + def as_ast_node(self) -> ast.Attribute | ast.Call: if not self.ut_method.has_args(): return self._attr_node @@ - return ast.Call(func=self._attr_node, args=args, keywords=[]) + return ast.Call(func=self._attr_node, args=args, keywords=[])
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def _attr_node(self) -> ast.Attribute: | |
| return ast.Attribute(value=ast.Name(id=UT), attr=self.ut_method) | |
| def as_ast_node(self) -> ast.Attribute | ast.Call: | |
| if not self.ut_method.has_args(): | |
| return self._attr_node | |
| args = [] | |
| if self.cond: | |
| args.append(ast.parse(self.cond).body[0].value) | |
| args.append(ast.Constant(value=self._reason)) | |
| return ast.Call(func=self._attr_node, args=args, keywords=[]) | |
| @property | |
| def _attr_node(self) -> ast.Attribute: | |
| return ast.Attribute( | |
| value=ast.Name(id=UT, ctx=ast.Load()), | |
| attr=str(self.ut_method), | |
| ctx=ast.Load(), | |
| ) | |
| def as_ast_node(self) -> ast.Attribute | ast.Call: | |
| if not self.ut_method.has_args(): | |
| return self._attr_node | |
| args = [] | |
| if self.cond: | |
| args.append(ast.parse(self.cond).body[0].value) | |
| args.append(ast.Constant(value=self._reason)) | |
| return ast.Call(func=self._attr_node, args=args, keywords=[]) |
🤖 Prompt for AI Agents
In scripts/lib_updater.py around lines 91 to 105, the constructed AST nodes lack
ctx which breaks ast.unparse; update the node constructors to include
ctx=ast.Load() — set ast.Name(..., ctx=ast.Load()) and ast.Attribute(...,
ctx=ast.Load()) where created (and ensure any other manually-constructed AST
Name/Attribute in this function uses ctx=ast.Load()); leave
ast.Call/ast.Constant as-is and keep existing conditional arg parsing.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| # If our ut_method has args then, | ||
| # we need to search for a constant that contains our `COMMENT`. | ||
| # Otherwise we need to search it in the raw source code :/ | ||
| if ut_method.has_args(): | ||
| reason = next( | ||
| ( | ||
| node.value | ||
| for node in ast.walk(dec_node) | ||
| if isinstance(node, ast.Constant) | ||
| and isinstance(node.value, str) | ||
| and COMMENT in node.value | ||
| ), | ||
| None, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Harden reason/cond extraction against edge cases.
- Guard against missing args when
has_cond()is true to avoidIndexError. - Make the regex more robust and anchored to end-of-line to avoid over-capturing.
- Optionally accept f-strings (
ast.JoinedStr) as reasons.
Apply this diff:
- if ut_method.has_args(): + if ut_method.has_args(): reason = next( ( - node.value - for node in ast.walk(dec_node) - if isinstance(node, ast.Constant) - and isinstance(node.value, str) - and COMMENT in node.value + node.value + for node in ast.walk(dec_node) + if isinstance(node, ast.Constant) + and isinstance(node.value, str) + and COMMENT in node.value ), None, ) @@ - if ut_method.has_cond(): - cond = ast.unparse(dec_node.args[0]) + if ut_method.has_cond() and getattr(dec_node, "args", []): + cond = ast.unparse(dec_node.args[0]) else: # Search first on decorator line, then in the line before for line in lines[dec_node.lineno - 1 : dec_node.lineno - 3 : -1]: - if found := re.search(rf"{COMMENT}.?(.*)", line): - reason = found.group() + if found := re.search(rf"{re.escape(COMMENT)}\s*[:;,-]?\s*(.*)$", line): + reason = found.group() break else: # Didn't find our `COMMENT` :) continue - reason = reason.removeprefix(COMMENT).strip(";:, ") + reason = reason.removeprefix(COMMENT).strip(";:, ")
If you want f-strings support, we can extend the walker to accept ast.JoinedStr and convert via ast.unparse(node).
Also applies to: 174-181, 182-191
🤖 Prompt for AI Agents
In scripts/lib_updater.py around lines 159-173 (and similarly 174-181, 182-191),
the extraction of reason/cond is brittle: it can IndexError when args are
missing, the regex may over-capture, and f-strings (ast.JoinedStr) are not
handled. Fix by first checking ut_method.has_args() and also guarding
ut_method.args list length before indexing; when has_cond() is true verify args
exist before accessing and return None if missing; tighten the regex to anchor
the match to end-of-line (e.g., use a pattern that requires COMMENT followed by
optional whitespace and end-of-line) to avoid over-capturing; and extend the AST
walker to accept ast.JoinedStr nodes (convert them via ast.unparse(node)) in
addition to ast.Constant strings so f-strings are supported; apply the same
guards and parsing changes to the other two blocks at lines 174-181 and 182-191.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.