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_codegen): use doc comments instead of insert! #4777

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented Aug 9, 2024

Avoid the insert! macro in AST codegen. Use doc comments starting with special symbol @ instead.

  • Before: insert!("// plain comment");
  • After: ///@ plain comment
  • Or: //!@ plain comment

Either ///@ or //!@ is converted to plain // in output.

//!@ is legal in top-of-file position, which allows us to inline #![allow(...)] attributes, which in my opinion makes the generators a bit easier to read.

Copy link

graphite-app bot commented Aug 9, 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.

@overlookmotel
Copy link
Collaborator Author

@rzvxa The @ symbol is a bit mysterious, so not ideal, but on balance I think it's preferable to the harder-to-read insert!(). What do you think?

Copy link

codspeed-hq bot commented Aug 9, 2024

CodSpeed Performance Report

Merging #4777 will not alter performance

Comparing 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ (f418f62) with main (b22ed45)

Summary

✅ 29 untouched benchmarks

@rzvxa rzvxa force-pushed the 08-09-refactor_ast_codegen_move_formatting_regex_definitions branch from 1266d4d to 09d8aec Compare August 9, 2024 07:18
@rzvxa rzvxa force-pushed the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch from d5f689c to ee406fa Compare August 9, 2024 07:19
@rzvxa
Copy link
Collaborator

rzvxa commented Aug 9, 2024

To be honest I don't like this change, It is too complicated for a simple thing.
I prefer the syntax noise over having too many symbols.

With that said, Feel free to merge as you wish.

@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_move_formatting_regex_definitions branch from 09d8aec to 3136858 Compare August 9, 2024 07:32
@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch from ee406fa to 7713818 Compare August 9, 2024 07:33
@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_move_formatting_regex_definitions branch from 3136858 to 0b9a2eb Compare August 9, 2024 07:39
@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch from 7713818 to ef525ec Compare August 9, 2024 07:39
@rzvxa rzvxa changed the base branch from 08-09-refactor_ast_codegen_move_formatting_regex_definitions to graphite-base/4777 August 9, 2024 07:49
@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch from ef525ec to bb2cd39 Compare August 9, 2024 07:49
@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch from bb2cd39 to 1605fa1 Compare August 9, 2024 07:52
@rzvxa rzvxa changed the base branch from graphite-base/4777 to main August 9, 2024 07:55
@overlookmotel
Copy link
Collaborator Author

To be honest I don't like this change, It is too complicated for a simple thing. I prefer the syntax noise over having too many symbols.

With that said, Feel free to merge as you wish.

OK. My feeling is that it's worth it for making generators/visit.rs clearer.

Since you've given me permission to merge, I'm going to do that.

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Aug 9, 2024
Copy link

graphite-app bot commented Aug 9, 2024

Merge activity

  • Aug 9, 4:18 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 9, 4:18 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
  • Aug 9, 4:22 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.

Avoid the `insert!` macro in AST codegen. Use doc comments starting with special symbol `@` instead.

* Before: `insert!("// plain comment");`
* After: `///@ plain comment`
* Or: `//!@ plain comment`

Either `///@` or `//!@` is converted to plain `//` in output.

`//!@` is legal in top-of-file position, which allows us to inline `#![allow(...)]` attributes, which in my opinion makes the generators a bit easier to read.
@overlookmotel overlookmotel force-pushed the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch from 1605fa1 to f418f62 Compare August 9, 2024 08:19
@graphite-app graphite-app bot merged commit f418f62 into main Aug 9, 2024
24 checks passed
@graphite-app graphite-app bot deleted the 08-09-refactor_ast_codegen_use_doc_comments_instead_of_insert_ branch August 9, 2024 08:22
overlookmotel added a commit that referenced this pull request Aug 9, 2024
Similar to #4777.

Use `///@@` instead of `endl!();` in AST codegen to create line breaks.

NB: `///@@` needs to be before an item, not after it.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants