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

Enforce trailing commas in object and collection initializer #668

Closed
NonSpicyBurrito opened this issue May 17, 2022 · 16 comments
Closed

Enforce trailing commas in object and collection initializer #668

NonSpicyBurrito opened this issue May 17, 2022 · 16 comments
Labels
Milestone

Comments

@NonSpicyBurrito
Copy link

Take the following code:

var foos = new[]
{
    new Foo(),
    new Bar()
};

If later a new item is added:

var foos = new[]
{
    new Foo(),
    new Bar(),
    new Baz()
};

This causes the following issues:

  • A misleading commit diff of +2/-1 even though there isn't really any code deletion.
  • Polluting git blame.

These issues also occur when deleting/reordering the last item in initializers.

These issues can be fixed by enforcing trailing commas.
Relevant Prettier option: https://prettier.io/docs/en/options.html#trailing-commas

@belav
Copy link
Owner

belav commented May 18, 2022

I had considered this early on - #23. I think it was closed because we decided to not have csharpier changing anything besides whitespace. But there are good arguments for transforming code in some cases, and there are good arguments for forcing tailing commas. The discussions I found around making trailing commas the default in prettier - prettier/prettier#9369 and prettier/prettier#68

I think I am in favor of adding this. If it were to follow the prettier way, it would only include the trailing comma when the initializer breaks.

@belav belav added type:enhancement New feature or request area:formatting labels May 18, 2022
@jods4
Copy link

jods4 commented Jul 24, 2023

Conversely, it would also be nice if CSharpier removed the trailing comma when the object initializer fits on single line.

I think it's a common style guide to not have the final comma on a single line, and include it on multiple lines, i.e. this format:

Dictionary<string, string> headers;

// No trailing comma after If-Match
headers = new()  { { "OData-Version", "4.0" }, { "If-Match", "*" } };

// Trailing comma after If-Match
headers = new() 
{
  { "OData-Version", "4.0" },
  { "Accept", "application/json;odata.metadata=minimal" },
  { "If-Match", "*" },
};

I think this makes a strong argument for CSharpier adding/removing that comma automatically:

  • Depending on the length of initializers, CSharpier will reflow on single or multiple lines.
  • Currently if you want to adhere to the style above, you need to manually add/remove that comma based on CSharpier reflow, so after CSharpier runs.
  • This is not doable if you run CSharpier automatically, e.g. as part of a git commit hook.
  • Even if you Format on Save, it's easy to miss.

@tkukushkin
Copy link

Also csharpier could act as Black (popular python code formatter) about trailing commas. It forces multiline output if sees trailing comma.
So this code in python

def foo(a: int, b: str,) -> None:

will be formatted to

def foo(
    a: int,
    b: str,
) -> None

And vice versa this code

def foo(
    a: int,
    b: str
) -> None:

will be formatted to

def foo(a: int, b: str) -> None:

because it doesn't have trailing comma and statement fits to one line.

It is really useful behaviour and gives power to users to force multiline output when it improves readability.

@NonSpicyBurrito
Copy link
Author

@tkukushkin I'm personally against that. The biggest benefit of Prettier is to simply stop people from arguing about style and just get things done. Introducing choices especially to the degree of being able to vary place by place, will devolve it back into arguing about when should we break into multiple lines or not.

@tkukushkin
Copy link

I understand your point and I can say from my experience, that I've never seen any arguments over someone splitting one statement into several lines when it fits into one. Nobody cares about such things. But this is my personal experience and maybe your experience is different.

I think Black was created with the same principle in mind, only few basic configuration options, it is uncompromising, opinionated. And this behaviour works for Black, then why wouldn't it work for csharpier?

@NonSpicyBurrito
Copy link
Author

NonSpicyBurrito commented Jul 29, 2023

I'm not sure that's a good reasoning, on the flip side "Prettier doesn't allow you the force a line split at all, why wouldn't it work for CSharpier?"
It's one less thing to worry about: if the line was too long when I originally wrote it, formatter should split it and insert the trailing comma; if later a parameter or two was removed and the line is no longer too long to fit in one line, formatter should just change it back to one line and also remove that trailing comma in the process, rather than keeping it split. The fact that you can arrive at the same code in two different ways and end up with two different formatting, is imo not desirable.

@jods4
Copy link

jods4 commented Jul 29, 2023

I'm gonna side with @NonSpicyBurrito on this one.

I like that CSharpier (or Prettier) handles like length for me. Managing line breaks manually just feels wrong.

Consider: after I (globally) rename a type in my solution, some method declaration may have significantly changed length.
I don't want to review each one manually to make them (un-)wrap based on the last comma.
I want CSharpier to just reformat them appropriately.

@tkukushkin
Copy link

Consider: after I (globally) rename a type in my solution, some method declaration may have significantly changed length.
I don't want to review each one manually to make them (un-)wrap based on the last comma.

If new name is longer and some statement (method call for example) doesn't fit in one line anymore, it will be formatted as multiline, no changes to current behaviour here.

If new name is shorter and some statement is already multiline, has trailing comma and could fit in one line, it will not be unwrapped, yes. And it is not common case IMHO, I have never reviewed each change manually, because it is not so important. There is no problem for me that some statements remain multiline.

I think we can stop arguing about this, I understand your point and respect your decision.

@Sahasrara
Copy link

It would be really nice to have this as a configuration option.

@belav belav added this to the 0.29.0 milestone Jun 16, 2024
belav added a commit that referenced this issue Jun 16, 2024
This pull request addresses the issue described in [Issue
#668](#668) and [Issue #1285
](#1285).
A flag UsePrettierStyleTrailingCommas is introduced which ensures a
trailing comma for anonymous object/collection/initializer/switch
expressions, list patterns and enum declarations in the case a line
break occurs.

I hope the implementation of the feature isn't too invasive. Obviously
it would be easier to implement the feature non-configurable.

I'm not sure how to deal with the tests, for now I enabled the flag and
adjusted the corresponding tests.

Thanks for any feedback!

---------

Co-authored-by: Bela VanderVoort <bela.vandervoort@optimizely.com>
@belav belav closed this as completed Jun 28, 2024
@martinib77
Copy link

It would be really nice to have this as a configuration option.

Is it posible to turn off this feature ? I really thinks it downgrades the quality of source the code

@belav
Copy link
Owner

belav commented Aug 17, 2024

Is it posible to turn off this feature ? I really thinks it downgrades the quality of source the code

@martinib77 there will not be a configuration option added for this. Can you elaborate why you don't like it? There are plenty of arguments for why they should be included but I have not come across any against it.

@martinib77
Copy link

Is it posible to turn off this feature ? I really thinks it downgrades the quality of source the code

@martinib77 there will not be a configuration option added for this. Can you elaborate why you don't like it? There are plenty of arguments for why they should be included but I have not come across any against it.

Sorry, code quality was not the best sentence. I wanted to refer to something more abstract, about the beauty and presentation of the code. Sentences that end in a comma and then close the block do not seem very pleasant to me.

@Meowtimer
Copy link

Is it posible to turn off this feature ? I really thinks it downgrades the quality of source the code

@martinib77 there will not be a configuration option added for this. Can you elaborate why you don't like it? There are plenty of arguments for why they should be included but I have not come across any against it.

Could it be argued that since prettier has an option for that (trailingComma) csharpier should have, too?

Sorry, code quality was not the best sentence. I wanted to refer to something more abstract, about the beauty and presentation of the code. Sentences that end in a comma and then close the block do not seem very pleasant to me.

I share the feeling :(

@NonSpicyBurrito
Copy link
Author

Could it be argued that since prettier has an option for that (trailingComma) csharpier should have, too?

From my understanding, the reason Prettier has an option for trailingComma is mainly because of the need to support old versions of JS where trailing comma is not legal syntax, and not necessarily because community opinion on it is split to a point where an option must be provided (like with tabs vs spaces, 2 vs 4, etc). See Prettier's 3.0 release blog post where trailing comma default value changed from es5 to all, the PR prettier/prettier#11479 associated with the change, and the prior PR prettier/prettier#6963 which changed from none to es5.
I personally wouldn't care if an option was added since I'd leave it enabled either way, but I think "Prettier has it so CSharpier should also have it" is not necessarily a strong point, because JS and C# are fundamentally different in that you do not control the language version in user's browser.

@alastair-todd
Copy link

Is it posible to turn off this feature ? I really thinks it downgrades the quality of source the code

@martinib77 there will not be a configuration option added for this. Can you elaborate why you don't like it? There are plenty of arguments for why they should be included but I have not come across any against it.

I just noticed csharpier doing this and it really got my back up.

Prettier has a config for this and our project has it turned off as trailing commas are gross for anyone with any sense of tidyness!

@jods4
Copy link

jods4 commented Aug 27, 2024

trailing commas are gross for anyone with any sense of tidyness!

Somehow many people, who lack any sense of tidyness apparently, like them. Including Prettier authors.

One key philosophy of Prettier/CSharpier is to remove style discussion in project teams, by not providing options.
This trailing comma is such a small thing that I'm sure you can live with it and it doesn't warrant an option.
(There's a good legacy reason for Prettier having this option, see comments above.)

Personally, there are other formatting choices from Prettier that I find way worse than a trailing comma, yet I just learnt to accept them and go along. Why? because overall the formatting is good, I like not having to bother with formatting and I value consistency in projects. Convenience wins over personal nitpicks.

I think that if you want full control over tiny details like trailing commas, CSharpier might not be the right tool for you.
Maybe look into using dotnet-format instead: everything is fully configurable.

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

No branches or pull requests

8 participants