Skip to content
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.

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

refactor(ast, transformer)!: remove BlockStatement::new methods #6783

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Oct 22, 2024

First of a series of PRs removing new and new_with_scope_id etc methods from AST types. Following #6760, all AST node creation can now go via the AST builder.

This lays groundwork for Node IDs (#5689), as we'll need NodeIds to be generated in AstBuilder, so that all nodes receive an ID.

Copy link

graphite-app bot commented Oct 22, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

@github-actions github-actions bot added A-ast Area - AST A-transformer Area - Transformer / Transpiler labels Oct 22, 2024
@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Oct 22, 2024
@overlookmotel overlookmotel changed the title refactor(ast, transformer): remove BlockStatement::new methods refactor(ast, transformer)!: remove BlockStatement::new methods Oct 22, 2024
@overlookmotel overlookmotel marked this pull request as ready for review October 22, 2024 17:05
@overlookmotel overlookmotel requested a review from Boshen October 22, 2024 17:08
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Oct 22, 2024

@Boshen I imagine something in this stack will break Rolldown. I assume they have access to AstBuilder, so it'll be an easy fix to migrate over to using AstBuilder methods to generate AST nodes?

Copy link

codspeed-hq bot commented Oct 22, 2024

CodSpeed Performance Report

Merging #6783 will not alter performance

Comparing 10-22-refactor_ast_transformer_remove_blockstatement_new_methods (2bee4e2) with main (8f17953)

Summary

✅ 30 untouched benchmarks

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Oct 23, 2024 — with Graphite App
Copy link

graphite-app bot commented Oct 23, 2024

Merge activity

)

First of a series of PRs removing `new` and `new_with_scope_id` etc methods from AST types. Following #6760, all AST node creation can now go via the AST builder.

This lays groundwork for Node IDs (#5689), as we'll need `NodeId`s to be generated in `AstBuilder`, so that all nodes receive an ID.
@Boshen Boshen force-pushed the 10-22-refactor_ast_transformer_remove_blockstatement_new_methods branch from 38530e2 to 2bee4e2 Compare October 23, 2024 03:36
@graphite-app graphite-app bot merged commit 2bee4e2 into main Oct 23, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 10-22-refactor_ast_transformer_remove_blockstatement_new_methods branch October 23, 2024 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-ast Area - AST A-transformer Area - Transformer / Transpiler C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants