rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

Fix handling of attribute in enum

Open malikolivier opened this issue 1 year ago • 3 comments

Fix #5662.

Previously, an enum item containing an attribute (rustdoc or macro for example) would be considered multi-line, thus forcing the formatting strategy of all the items in the enum to be Vertical (i.e. multi-line formatting).

This PR does the following:

  • Fixes the issue (ce990c80) and gate the changes (103d13e0, 56014626737e25e7d9a3e2e12f656231d5dce761)
  • Handle additional edge-cases involving attributes (277cb903, 3d1b831a)
    • There may be a better (shorter?, by using an appropriate lib) for an implementation, but could not figure it out as now. I believe this implementation works. If the maintainer believes code should be moved, re-architected in any way, please feel free to give feedback.
  • Add exhaustive tests (6f204cd4, df5b6d9e, 933e997a)
  • Fix rustfmt's formatting after the issue is fixed (757e73d7, 22b076c1)

TODO

  • [x] Multi-line macro attributes #[...] are not handled properly (done in 277cb903)

  • [x] https://www.github.com/rust-lang/rustfmt/issues/5662#issuecomment-1386231624 mentions that a new config option would be needed to allow both single line and multi-line enum variants. I am not sure to understand why. Given some guidance from the maintainer, I can certainly add a new config option in this PR.

malikolivier avatar Aug 19 '24 06:08 malikolivier

https://www.github.com/rust-lang/rustfmt/issues/5662#issuecomment-1386231624 mentions that a new config option would be needed to allow both single line and multi-line enum variants. I am not sure to understand why. Given some guidance from the maintainer, I can certainly add a new config option in this PR.

It's been a while since I made that comment but if I'm remembering correctly the idea with the new configuration was to allow mult-line and single-line variants to co-exist in an enum definition. Currently we'll try to rewrite all variants as multi-line if any of them are multi-line. I don't think it's necessary to add that option, but hopefully that gives you some additional context.

ytmimi avatar Aug 19 '24 19:08 ytmimi

Thank you for the review!

https://www.github.com/rust-lang/rustfmt/issues/5662#issuecomment-1386231624 mentions that a new config option would be needed to allow both single line and multi-line enum variants. I am not sure to understand why. Given some guidance from the maintainer, I can certainly add a new config option in this PR.

It's been a while since I made that comment but if I'm remembering correctly the idea with the new configuration was to allow mult-line and single-line variants to co-exist in an enum definition. Currently we'll try to rewrite all variants as multi-line if any of them are multi-line. I don't think it's necessary to add that option, but hopefully that gives you some additional context.

Thank you! I too believe the current approach of not having multi-line and single-line variants co-exist in an enum definition is all right. So I won't add any option.

I know you mentioned that multi-line attributes are a problem for the current approach. I'd prefer if we tried to resolve that issue before moving forward.

Got it! Will attempt to have a complete fix with no edge-case left ;) Depending on the amount of changes, I may close this PR and re-open a new one.

malikolivier avatar Aug 19 '24 23:08 malikolivier

I'd prefer if we tried to resolve that issue before moving forward.

@ytmimi I pushed code to the PR that resolves the issue (multi-line macro attributes: 277cb903, documentation block: 3d1b831a). Can you review again when you are available?

(at your request, I can rebase. This time, for easier review, I've added a few commits to fix the mentioned issues)

malikolivier avatar Aug 21 '24 04:08 malikolivier