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

fix(ast): rename serialized fields to camel case #2566

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Mar 2, 2024

Fixes a few more AST fields names which are currently snake case in serialized JSON.

Copy link
Contributor Author

overlookmotel commented Mar 2, 2024

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

Join @overlookmotel and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added the A-ast Area - AST label Mar 2, 2024
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Mar 2, 2024

@ArnaudBarre Was there a reason why you didn't do these already in #2522?

(trailing_comma in ArrayExpression, ObjectExpression and
ArrayAssignmentTarget)

Copy link

codspeed-hq bot commented Mar 2, 2024

CodSpeed Performance Report

Merging #2566 will not alter performance

Comparing 03-02-fix_ast_rename_serialized_fields_to_camel_case (c75a78e) with main (78f8c2c)

Summary

✅ 27 untouched benchmarks

@Boshen Boshen merged commit e339461 into main Mar 2, 2024
20 checks passed
@Boshen Boshen deleted the 03-02-fix_ast_rename_serialized_fields_to_camel_case branch March 2, 2024 14:47
@ArnaudBarre
Copy link
Contributor

ArnaudBarre commented Mar 2, 2024

I didn't include this one because I think tailing comma part of ESTree and I let discussing skip vs camelCase for later

@overlookmotel
Copy link
Contributor Author

Ah I see. Well maybe they should be skipped. But regardless of whether they're in ESTree or not, I think it's better all properties in JSON AST are camelCase - snake_case would be unexpected to JS users.

@ArnaudBarre
Copy link
Contributor

Yeah this could be added to all nodes for potential future field if the cost is 0, if there is a small cost I think this is fine adding it when needed

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Mar 2, 2024

I don't think there's any runtime cost to field renaming, as the serializers are built at compile-time. Maybe yes we should add to all AST structs.

a-rustacean pushed a commit to a-rustacean/oxc that referenced this pull request Mar 4, 2024
Fixes a few more AST fields names which are currently snake case in
serialized JSON.
a-rustacean added a commit to a-rustacean/oxc that referenced this pull request Mar 4, 2024
a-rustacean added a commit to a-rustacean/oxc that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants