Skip to content

Conversation

@ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Jan 29, 2022

Fixes #5193

The issues was related to the interaction between two differnt
configuration options when applied to the last match arm.

  • trailing_comma = Never
  • match_block_trailing_comma = true

Previously, rustfmt only consider the match_block_trailing_comma
configuration value when wrapping match arms. This lead to rustfmt
adding a trailing comma to all newly wrapped match arms.

Now, rustfmt also considers the trailing_comma configuration value
when deciding whether or not to add a trailing comma when wrapping the
last match arm in a block.

@ytmimi
Copy link
Contributor Author

ytmimi commented Jan 29, 2022

@calebcartwright Two things to note:

  1. I plan on adding some additional tests cases to cover the various combinations of trailing_comma and match_block_trailing_comma. Just wanted to get the PR in when I identified an initial solution.

  2. I know you mentioned that #5193 might be a duplicate. If you happen to track down the original issue (or other duplicates), I'll see if this PR fixes them as well and add test cases accordingly!

@calebcartwright
Copy link
Member

Changes LGTM @ytmimi, and happy to merge as-is. Will hold off on merging since it sounds like there may be more you want to do, but lmk when you're ready

@ytmimi
Copy link
Contributor Author

ytmimi commented Feb 7, 2022

@calebcartwright Awesome! Yeah, I'd just want to add a few more test cases and see if I can track down any duplicates. I'll make sure to let you know when that's all taken care of!

The issues was related to the interaction between two differnt
configuration options when applied to the last match arm.

* trailing_comma = Never
* match_block_trailing_comma = true

Previously, rustfmt only consider the ``match_block_trailing_comma``
configuration value when wrapping match arms. This lead to rustfmt
adding a trailing comma to all newly wrapped match arms.

Now, rustfmt also considers the ``trailing_comma`` configuration value
when deciding whether or not to add a trailing comma when wrapping the
last match arm in a block.
Comment on lines +12 to +14
FooBar::Baz => {
println!("Lorem ipsum dolor sit amet, consectetuer adipiscing elit.")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it a little odd that trailing_comma=Never takes precedence over match_block_trailing_comma=true, for the last match arm but that's what I needed to do in order to prevent the idempotency issue since that's how arm_comma is implemented.

rustfmt/src/matches.rs

Lines 146 to 160 in a67d909

fn arm_comma(config: &Config, body: &ast::Expr, is_last: bool) -> &'static str {
if is_last && config.trailing_comma() == SeparatorTactic::Never {
""
} else if config.match_block_trailing_comma() {
","
} else if let ast::ExprKind::Block(ref block, _) = body.kind {
if let ast::BlockCheckMode::Default = block.rules {
""
} else {
","
}
} else {
","
}
}

@ytmimi
Copy link
Contributor Author

ytmimi commented Aug 9, 2022

@calebcartwright no rush on this. I know it's been a while since you've last reviewed it. The last time you looked at these changes you said it was ready to merge, but since I've added some additional test cases I feel like it would be worth it to do another review if we choose to move forward with this

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.

My code needs two passes to be formatted correctly

2 participants