Skip to content

refactor(transformer/using): remove use of child scope ids in enter_program #9748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 13, 2025
Merged

refactor(transformer/using): remove use of child scope ids in enter_program #9748

merged 1 commit into from
Mar 13, 2025

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Mar 13, 2025

No description provided.

@camc314 camc314 marked this pull request as ready for review March 13, 2025 10:29
@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Mar 13, 2025
@camc314 Graphite App
Copy link
Contributor Author

camc314 commented Mar 13, 2025 •
edited
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camc314 camc314 requested a review from Boshen March 13, 2025 10:30
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Mar 13, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #9748 will not alter performance

Comparing c/03-13-refactor_transformer_using_remove_use_of_child_scope_ids_in_enter_program (43e8efd) with main (8dd6809)

Summary

✅ 33 untouched benchmarks

@camc314 camc314 force-pushed the c/03-13-refactor_transformer_using_remove_use_of_child_scope_ids_in_enter_program branch from ba645ed to 6e05956 Compare March 13, 2025 11:25
@camc314 camc314 changed the base branch from main to graphite-base/9748 March 13, 2025 11:48
@camc314 camc314 force-pushed the c/03-13-refactor_transformer_using_remove_use_of_child_scope_ids_in_enter_program branch from 6e05956 to f495bb3 Compare March 13, 2025 11:48
@camc314 camc314 changed the base branch from graphite-base/9748 to 03-12-test_transformer_using_add_failing_test_for_scopes March 13, 2025 11:48
This was referenced Mar 13, 2025
Merged
Merged
Base automatically changed from 03-12-test_transformer_using_add_failing_test_for_scopes to main March 13, 2025 11:59
overlookmotel
overlookmotel approved these changes Mar 13, 2025
View reviewed changes
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, this change is correct. It's changing the parent of the right scopes correctly.

However, it is introducing another loop over statements. We could expose ChildScopeCollector and then this transform could use it directly, calling visit_statement on it while it's already looping over the statements.

Let's merge this now, and then we can do that in a follow-up PR.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Mar 13, 2025
@overlookmotel Graphite App
Copy link
Member

overlookmotel commented Mar 13, 2025 •
edited by graphite-app bot
Loading

Merge activity

  • Mar 13, 9:25 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Mar 13, 9:25 AM EDT: A user added this pull request to the Graphite merge queue.
  • Mar 13, 9:31 AM EDT: A user merged this pull request with the Graphite merge queue.

@graphite-app graphite-app bot force-pushed the c/03-13-refactor_transformer_using_remove_use_of_child_scope_ids_in_enter_program branch from f495bb3 to 43e8efd Compare March 13, 2025 13:26
@graphite-app graphite-app bot merged commit 43e8efd into main Mar 13, 2025
26 checks passed
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 13, 2025
@graphite-app graphite-app bot deleted the c/03-13-refactor_transformer_using_remove_use_of_child_scope_ids_in_enter_program branch March 13, 2025 13:31
This was referenced Mar 13, 2025
Merged
Merged
graphite-app bot pushed a commit that referenced this pull request Mar 13, 2025
@overlookmotel
refactor(traverse): add doc comment for `TraverseScoping::insert_scop…
d7dda5c 
…e_below_statements` and rename var (#9756)

Follow-on after #9748. Document the `TraverseScoping::insert_scope_below_statements` method added in that PR. Rename a variable to a more appropriate name.
graphite-app bot pushed a commit that referenced this pull request Mar 13, 2025
@overlookmotel
refactor(traverse): add TraverseCtx::insert_scope_below_statements (#…
bda4b9a 
…9757)

Follow-on after #9748. Our convention is to add aliases for all methods on `TraverseScoping` to `TraverseCtx` too, to make usage shorter.

Add an alias for `insert_scope_below_statements`, added in #9748.
@oxc-bot oxc-bot mentioned this pull request Mar 18, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@overlookmotel overlookmotel overlookmotel approved these changes

@Dunqing Dunqing Awaiting requested review from Dunqing Dunqing is a code owner

@Boshen Boshen Awaiting requested review from Boshen

Assignees
No one assigned
Labels
A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@camc314 @overlookmotel