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 — Fix sub_table ordering for nested inlined comprehensions (PEP 709) by LegNeato · Pull Request #7480 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughReorders compilation sequence in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can disable the changed files summary in the walkthrough.Disable the |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
🧹 Nitpick comments (1)
extra_tests/snippets/syntax_comprehension.py (1)
50-81: Ensure this snippet suite is executed in debug mode in CI/test docs.These additions are in
extra_tests/snippets/**; keeping execution oncargo run(debug) avoids unnecessary compile-time overhead for this suite.As per coding guidelines, "
extra_tests/snippets/**: Use debug mode (cargo run) forextra_tests/snippetstests as compilation is faster".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extra_tests/snippets/syntax_comprehension.py` around lines 50 - 81, Update CI/test documentation and the test-runner config so the extra_tests/snippets suite runs in debug mode (via cargo run) instead of a release/compiled invocation; specifically ensure any CI job or test script that executes tests under the directory extra_tests/snippets uses cargo run (debug) to invoke these snippet files (including test_nested_comp_with_lambda, test_nested_comp_underscore, test_simple_nested_comp) to avoid unnecessary compile-time overhead. Adjust the relevant CI step or README/test docs to call cargo run for that path and confirm the change is applied where extra_tests/snippets is referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extra_tests/snippets/syntax_comprehension.py`:
- Around line 50-81: Update CI/test documentation and the test-runner config so
the extra_tests/snippets suite runs in debug mode (via cargo run) instead of a
release/compiled invocation; specifically ensure any CI job or test script that
executes tests under the directory extra_tests/snippets uses cargo run (debug)
to invoke these snippet files (including test_nested_comp_with_lambda,
test_nested_comp_underscore, test_simple_nested_comp) to avoid unnecessary
compile-time overhead. Adjust the relevant CI step or README/test docs to call
cargo run for that path and confirm the change is applied where
extra_tests/snippets is referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 51cda9eb-baf9-4d88-bc93-6fa42c52ff77
📒 Files selected for processing (2)
crates/codegen/src/compile.rsextra_tests/snippets/syntax_comprehension.py
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
youknowone
left a comment
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.
Thank you for contributing! You don't need to worry about AI develop as long as you are in the loop.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
…7480) When an inlined comprehension's first iterator expression contains nested scopes (such as a lambda), those scopes' sub_tables appear at the current position in the parent's sub_table list. The previous code spliced the comprehension's own child sub_tables (e.g. inner inlined comprehensions) into that same position before compiling the iterator, which shifted the iterator's sub_tables to wrong indices. Move the splice after the first iterator is compiled so its sub_tables are consumed at their original positions. Fixes nested list comprehensions like: ```python [[x for _, x in g] for _, g in itertools.groupby(..., lambda x: ...)] ``` Disclosure: I used AI to develop the patch though I was heavily involved.
…7480) When an inlined comprehension's first iterator expression contains nested scopes (such as a lambda), those scopes' sub_tables appear at the current position in the parent's sub_table list. The previous code spliced the comprehension's own child sub_tables (e.g. inner inlined comprehensions) into that same position before compiling the iterator, which shifted the iterator's sub_tables to wrong indices. Move the splice after the first iterator is compiled so its sub_tables are consumed at their original positions. Fixes nested list comprehensions like: ```python [[x for _, x in g] for _, g in itertools.groupby(..., lambda x: ...)] ``` Disclosure: I used AI to develop the patch though I was heavily involved.
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.