Preserve trailing commas for enums with members when trailing_commas are preserved #1703
Conversation
|
@munificent I saw your recent changes guarding the formatter with a language version flag. Please advise if the changes here require any additional modifications. FYI, I pulled in the new changes, and all ran successfully. |
|
@munificent Have you had a change to go over the PR, yet? |
|
@munificent Is there anything else I should do here? |
|
@munificent Any plans to review this in the near future? Should I add a changelog entry? |
munificent
changed the base branch from
main
to
preserve-trailing-commas-enum-with-members
|
Sorry for the long delay. After getting 3.8 out the door, I needed to take a break from working on the formatter and get caught up on other stuff. I really like this change. Thank you for the PR! We're moving to a model where style changes are language versioned. I'll take care of handling that. So I'll merge this into a branch instead of main and then apply the language versioning change on top of that before merging into main. I hope that's OK. I'll also take care of adding this to the CHANGELOG. |
munificent
merged commit b02717b
into
dart-lang:preserve-trailing-commas-enum-with-members
|
Thank you! |
Preserve trailing commas for enums with members when commas are preserved (#1703) This PR changes the formatter to have it maintain trailing commas of last enum constants when there are members. So: ```dart enum E { a, b, c,; int x; } ``` Becomes: ```dart enum E { a, b, c, ; int x; } ``` Instead of: ```dart enum E { a, b, c; int x; } ``` Fixes #1678 Fixes #1729 Additionally, a similar style choice was presented in #1660 (comment) **Affected users**: - Those passing the `trailing_commas: preserve` config option and adding a trailing comma to their enum cases. - Users who don't already pass a trailing comma, aren't affected. So this PR shouldn't have undesirable effects on existing users. **Benefits**: - Maintains developer's intent - Less churn when adding additional member cases (Personally, I always add a trailing semicolon to avoid churn and to also make it even clearer this is an advanced enum with members.) **Downsides**: - 7 additional logical lines of code to maintain **Additional considerations**: I also considered implementing this for `enum E { a, b,; }`, however, that would require more drastic changes to the formatter as far as I can tell. Additionally, in this case the trailing semicolon provides fewer benefits: - churn when adding new members is the same if there was a trailing semicolon or not - giving the enum other members also only amounts to new lines being added rather than changed so again, no real churn. **Notes** - One test was changed due to this being a change in behavior. - This was surprisingly easy and enjoyable to implement (compliment to the existing code base). - I didn't make any changes to the `CHANGELOG.md` since this trailing commas preservation hasn't shipped yet so its behaviour is still undefined. Co-authored-by: jellynoone <[email protected]>
I saw this change happening. I considered adding the check but, I figured this was a non-breaking change that it might make sense to always apply since the only people affected are people running |
This PR changes the formatter to have it maintain trailing commas of last enum constants when there are members.
So:
Becomes:
Instead of:
Fixes #1678
Fixes #1729
Additionally, a similar style choice was presented in #1660 (comment)
Affected users:
trailing_commas: preserveconfig option and adding a trailing comma to their enum cases.Benefits:
(Personally, I always add a trailing semicolon to avoid churn and to also make it even clearer this is an advanced enum with members.)
Downsides:
Additional considerations:
I also considered implementing this for
enum E { a, b,; }, however, that would require more drastic changes to the formatter as far as I can tell.Additionally, in this case the trailing semicolon provides fewer benefits:
Notes
CHANGELOG.mdsince this trailing commas preservation hasn't shipped yet so its behaviour is still undefined.