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 additional semicolon during changing parentheses style #6024

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rscprof
Copy link

@rscprof rscprof commented Jan 12, 2024

#6013

Prevent add semicolon when changing parentheses style when using lazy_static. If we see that previous parentheses is round and we use the rule for lazy_static then we set flag about it and after it we simple skip insert semicolon during postprocessing spaces and semicolon.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rscprof thanks for helping out on this one. I'd like to take a more general approach to solve this. Instead of adding state to the context specifically for lazy_static!, I think it would be better if the call to rewrite_macro and rewrite_macro_inner returned (Option<String>, Option<Delimiter>). Then in FmtVisitor::visit_mac we can make appropriate changes to the span we pass to push_rewrite in order to inform rustfmt that we've rewritten the entirety of the old macro call including the trailing ;.

We'll need to update some call sites, but that shouldn't be a big deal. Something like:

- rewrite_macro(...)
+ let (rewrite, _) = rewrite_macro(...);

We'll also need to tweak with_context to be generic over its return type.

@rscprof
Copy link
Author

rscprof commented Jan 12, 2024

Hi @ytmimi, I thought about returning pair, but I thought that it changes functions signatures drastically. I will change it tomorrow in your way.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rscprof Thanks for your help! I finally had some time to get back around to reviewing this. I think your changes show that you understood what needed to be done to fix the issue. I think there were a few places where we could have made alternative code changes to simplify the diff. Also, I noticed in a few spots that you were conditionally returning the delimiters. I felt it was simpler to always return the rewritten delimiters if we could. Overall, good work so far, and I'd like to see the following changes.

src/expr.rs Outdated Show resolved Hide resolved
src/items.rs Outdated Show resolved Hide resolved
src/macros.rs Outdated Show resolved Hide resolved
src/macros.rs Outdated Show resolved Hide resolved
src/macros.rs Outdated Show resolved Hide resolved
src/patterns.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/visitor.rs Show resolved Hide resolved
src/visitor.rs Show resolved Hide resolved
src/visitor.rs Show resolved Hide resolved
@ytmimi
Copy link
Contributor

ytmimi commented Mar 28, 2024

@rscprof Can you take a look at the failures when you have a chance. Also, the rustfmt team strongly discourages merge commits. When you have a moment can you rebase these changes instead.

@rscprof
Copy link
Author

rscprof commented Mar 28, 2024

@rscprof Can you take a look at the failures when you have a chance. Also, the rustfmt team strongly discourages merge commits. When you have a moment can you rebase these changes instead.

Hi! I'm going to do all things in a week

@rscprof rscprof requested a review from ytmimi March 28, 2024 20:59
@ytmimi
Copy link
Contributor

ytmimi commented Mar 28, 2024

@rscprof I'm still seeing the Merge branch 'master' into issue-6013 commit in the history. Can you please update the git history to remove that merge commit.

@rscprof rscprof force-pushed the issue-6013 branch 2 times, most recently from 42466c8 to 25a9a5f Compare March 29, 2024 06:22
@rscprof
Copy link
Author

rscprof commented Mar 29, 2024

@rscprof I'm still seeing the Merge branch 'master' into issue-6013 commit in the history. Can you please update the git history to remove that merge commit.

I made one commit for radically simplifying history

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants