Skip to content

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Aug 8, 2024

Simplify derive_get_span generator that was introduced in #4735. No change to functionality, just aiming for greater readability.

In particular:

  • Move defining idents/tokens which are specific to GetSpan / GetSpanMut into those specific generators, rather than branching on MUT later on.
  • Remove const MUT param.
  • Remove the confusing pairs of closures and functions both called derive_enum / derive_struct.
  • Inline function which generates the impls - prioritizing readability over DRY code.

@graphite-app
Copy link
Contributor

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

Copy link
Member Author

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

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

@overlookmotel overlookmotel marked this pull request as ready for review August 8, 2024 09:33
@overlookmotel overlookmotel requested a review from rzvxa August 8, 2024 09:33
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 8, 2024

CodSpeed Performance Report

Merging #4757 will not alter performance

Comparing 08-08-refactor_ast_codegen_simplify_derive_get_span_generator (790c551) with main (2f6c3b9)

Summary

✅ 29 untouched benchmarks

@overlookmotel
Copy link
Member Author

What do you think @rzvxa? Personally I find this style less convoluted and a lot easier to understand.

NB: Rather than looking at the diff, I'd suggest comparing before and after.

@rzvxa
Copy link
Contributor

rzvxa commented Aug 8, 2024

What do you think @rzvxa? Personally I find this style less convoluted and a lot easier to understand.

TBH, I don't have a strong preference here. I'll merge it in if you like it more this way.

@rzvxa rzvxa added the 0-merge Merge with Graphite Merge Queue label Aug 8, 2024
@graphite-app
Copy link
Contributor

graphite-app bot commented Aug 8, 2024

Merge activity

  • Aug 8, 6:46 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Aug 8, 6:46 AM EDT: rzvxa added this pull request to the Graphite merge queue.
  • Aug 8, 6:50 AM EDT: rzvxa merged this pull request with the Graphite merge queue.

Simplify `derive_get_span` generator that was introduced in #4735. No change to functionality, just aiming for greater readability.

In particular:

* Move defining idents/tokens which are specific to `GetSpan` / `GetSpanMut` into those specific generators, rather than branching on `MUT` later on.
* Remove `const MUT` param.
* Remove the confusing pairs of closures and functions both called `derive_enum` / `derive_struct`.
* Inline function which generates the impls - prioritizing readability over DRY code.
@rzvxa rzvxa force-pushed the 08-08-refactor_ast_codegen_simplify_derive_get_span_generator branch from 1b9561b to 790c551 Compare August 8, 2024 10:47
@graphite-app graphite-app bot merged commit 790c551 into main Aug 8, 2024
@graphite-app graphite-app bot deleted the 08-08-refactor_ast_codegen_simplify_derive_get_span_generator branch August 8, 2024 10:50
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.

3 participants