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/ast_builder)!: shorter allocator utility method names. #4122

Conversation

rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Jul 8, 2024

This PR serves two purposes, First off it would lower the amount of characters we have to type in for a simple operation such as wrapping an expression in a vector. Secondly, it would follow the generated names more closely since nowhere else in the builder we do have new_xxx, We always say xxx since a builder always constructs something.

new_vec -> vec
new_vec_single -> vec1*
new_vec_from_iter -> vec_from_iter
new_vec_with_capacity -> vec_with_capacity
new_str -> str
new_atom -> atom

* This one is the main motivation behind this PR, It saves 10 characters!

Copy link

graphite-app bot commented Jul 8, 2024

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

Add the label “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-parser Area - Parser A-minifier Area - Minifier A-ast Area - AST A-transformer Area - Transformer / Transpiler A-isolated-declarations Isolated Declarations labels Jul 8, 2024
@rzvxa rzvxa marked this pull request as ready for review July 8, 2024 17:20
Copy link

codspeed-hq bot commented Jul 8, 2024

CodSpeed Performance Report

Merging #4122 will not alter performance

Comparing 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names (b936162) with main (e29cdbf)

Summary

✅ 29 untouched benchmarks

@rzvxa rzvxa force-pushed the 07-08-feat_ast_codegen_add_cli_arguments_to_use_with_oxc_ast_codegen_executable branch from a3735c5 to f211477 Compare July 8, 2024 22:28
@rzvxa rzvxa force-pushed the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch from 1a82128 to 451cf2e Compare July 8, 2024 22:28
@overlookmotel
Copy link
Contributor

I agree with the logic of this change. I'd suggest vec_from_iter rather than vec_from. That might be an abbreviation too far (from what?) - and also it's not much used.

@overlookmotel
Copy link
Contributor

overlookmotel commented Jul 9, 2024

I still prefer vec_from_iter as it matches std's naming convention (Vec::from_iter also accepts anything which can be iterated). We could conceivably want to add e.g. vec_from_slice later on. But I don't feel so strongly about it.

@rzvxa
Copy link
Contributor Author

rzvxa commented Jul 9, 2024

I agree with the logic of this change. I'd suggest vec_from_iter rather than vec_from. That might be an abbreviation too far (from what?) - and also it's not much used.

At first, I thought it was accepting iterators before so I sent a false comment. I removed it to write an accurate answer instead.

I chose this name because it doesn't accept Iterator, It accepts IntoIterator which can be a variety of things such as Some(T) so that's why I went with this generic name. Let me know if you don't follow my reasoning or if you have a relatively strong objection, I'm flexible for changing it.

@rzvxa
Copy link
Contributor Author

rzvxa commented Jul 9, 2024

I still prefer vec_from_iter as it matches std's naming convention (Vec::from_iter also accepts anything which can be iterated). We could conceivably want to add e.g. vec_from_slice later on. But I don't feel so strongly about it.

I agree with your reasoning, Just pushed a new commit addressing this.

@overlookmotel overlookmotel force-pushed the 07-08-feat_ast_codegen_add_cli_arguments_to_use_with_oxc_ast_codegen_executable branch from f211477 to 1afc128 Compare July 9, 2024 00:54
@overlookmotel overlookmotel force-pushed the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch from 05e145b to e69c323 Compare July 9, 2024 00:55
@overlookmotel overlookmotel force-pushed the 07-08-feat_ast_codegen_add_cli_arguments_to_use_with_oxc_ast_codegen_executable branch from 1afc128 to 9aa4b60 Compare July 9, 2024 01:09
@overlookmotel overlookmotel force-pushed the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch from e69c323 to 2943085 Compare July 9, 2024 01:10
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jul 9, 2024 — with Graphite App
Copy link

graphite-app bot commented Jul 9, 2024

Merge activity

  • Jul 8, 11:40 PM EDT: Boshen added this pull request to the Graphite merge queue.
  • Jul 8, 11:55 PM EDT: The Graphite merge queue couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'Format').
  • Jul 9, 8:12 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 9, 8:16 AM EDT: rzvxa added this pull request to the Graphite merge queue.
  • Jul 9, 8:20 AM EDT: rzvxa merged this pull request with the Graphite merge queue.

@Boshen Boshen force-pushed the 07-08-feat_ast_codegen_add_cli_arguments_to_use_with_oxc_ast_codegen_executable branch from 9aa4b60 to 0b43310 Compare July 9, 2024 03:43
Boshen pushed a commit that referenced this pull request Jul 9, 2024
…4122)

This PR serves two purposes, First off it would lower the amount of characters we have to type in for a simple operation such as wrapping an expression in a vector. Secondly, it would follow the generated names more closely since nowhere else in the builder we do have `new_xxx`, We always say `xxx` since a builder always constructs something.

```
new_vec -> vec
new_vec_single -> vec1*
new_vec_from_iter -> vec_from_iter
new_vec_with_capacity -> vec_with_capacity
new_str -> str
new_atom -> atom
```

`*` This one is the main motivation behind this PR, It saves 10 characters!
@Boshen Boshen force-pushed the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch from 2943085 to 69cea86 Compare July 9, 2024 03:44
@Boshen Boshen changed the base branch from 07-08-feat_ast_codegen_add_cli_arguments_to_use_with_oxc_ast_codegen_executable to main July 9, 2024 03:54
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jul 9, 2024
rzvxa added a commit that referenced this pull request Jul 9, 2024
…4122)

This PR serves two purposes, First off it would lower the amount of characters we have to type in for a simple operation such as wrapping an expression in a vector. Secondly, it would follow the generated names more closely since nowhere else in the builder we do have `new_xxx`, We always say `xxx` since a builder always constructs something.

```
new_vec -> vec
new_vec_single -> vec1*
new_vec_from_iter -> vec_from_iter
new_vec_with_capacity -> vec_with_capacity
new_str -> str
new_atom -> atom
```

`*` This one is the main motivation behind this PR, It saves 10 characters!
@rzvxa rzvxa force-pushed the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch from 69cea86 to c5a6e9e Compare July 9, 2024 12:12
@rzvxa rzvxa added the 0-merge Merge with Graphite Merge Queue label Jul 9, 2024
…4122)

This PR serves two purposes, First off it would lower the amount of characters we have to type in for a simple operation such as wrapping an expression in a vector. Secondly, it would follow the generated names more closely since nowhere else in the builder we do have `new_xxx`, We always say `xxx` since a builder always constructs something.

```
new_vec -> vec
new_vec_single -> vec1*
new_vec_from_iter -> vec_from_iter
new_vec_with_capacity -> vec_with_capacity
new_str -> str
new_atom -> atom
```

`*` This one is the main motivation behind this PR, It saves 10 characters!
@rzvxa rzvxa force-pushed the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch from c5a6e9e to b936162 Compare July 9, 2024 12:16
@graphite-app graphite-app bot merged commit b936162 into main Jul 9, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 07-08-refactor_ast_ast_builder_shorter_allocator_utility_method_names branch July 9, 2024 12:20
@github-actions github-actions bot mentioned this pull request Jul 9, 2024
Boshen added a commit that referenced this pull request Jul 9, 2024
## [0.19.0] - 2024-07-09

- b936162 ast/ast_builder: [**BREAKING**] Shorter allocator utility
method names. (#4122) (rzvxa)

### Features

- 485c871 ast: Allow conversion from `Expression` into `Statement` with
`FromIn` trait. (#4124) (rzvxa)

### Refactor

Co-authored-by: Boshen <Boshen@users.noreply.github.com>
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-isolated-declarations Isolated Declarations A-minifier Area - Minifier A-parser Area - Parser A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants