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

Add the :trailing_comma option to the formatter #7689

Closed
wants to merge 1 commit into from

Conversation

gamache
Copy link
Contributor

@gamache gamache commented May 16, 2018

This commit adds support for the :trailing_comma option in Code.format_string!/2 et al, including in .formatter.exs.

When set true, multi-line list, map, and struct literals will include a trailing comma after the last item or pair in the data structure.

Does not affect other data structures, or lists/maps/structs rendered on a single line.

--

The topic of trailing commas has been raised previously by @pragdave in #6646. No resolution was reached; instead, it was considered appropriate to wait for the formatter to have more time in common use before making decisions on which syntax variants to allow. It's now been a little over six months since the last discussion, and in conversations with Elixir developers, the lack of trailing commas continues to be a common source of friction.

Some possible reasons to prefer trailing commas in multi-line data structures are ease of refactoring, minimal diffs when adding or removing elements, and visual symmetry. These reasons are not worth discussing directly in this PR. It should suffice to note that there are advantages and disadvantages to either approach, and that many developers and teams feel strongly about their preference.

This PR's goal is not to dictate the style of the Elixir project or community at large. In a similar way to the existing :line_length option, the :trailing_comma option provides an opt-in mechanism by which projects and organizations can benefit from standardized code formatting while retaining a style they find to be more efficient and productive.

This commit adds support for the `:trailing_comma` option in
`Code.format_string!/2` et al, including in `.formatter.exs`.

When set `true`, multi-line list, map, and struct literals will
include a trailing comma after the last item or pair in the data
structure.

Does not affect other data structures, or lists/maps/structs rendered
on a single line.
@pmarreck
Copy link

I have been nothing but impressed with the formatter (which I honestly did not originally expect)... Except for this little detail! I love me some trailing commas for all the reasons cited. (Possibly related: I'm also a fan of the Oxford comma!)

In short, I think this would be a useful feature to add.

@josevalim
Copy link
Member

josevalim commented May 16, 2018

Hi @gamache! 👋

As you know, the goal of the formatter is exactly to unify the code style used by the community. While I understand this PR is an argument to relax this particular goal, the formatter has been publicly released for roughy 4 months. We need to get much more mileage and feedback before we can perform such changes.

Note we had long discussions in the past about formatter customization, such as #6647, and the conclusion there was the same: t is too soon in the formatter adoption cycle to warrant configuration options.

I also would like to note that the combination of optional parens and keyword lists make trailing commas slightly inconsistent in Elixir as the following code is ambiguous (and therefore disallowed):

some_fun :foo,
         key: bar,
baz

This was one of the main reasons for disallowing trailing commas in the formatter, as in an attempt to standardize a developer could end-up adding comma to unwarranted places.

For all of those reasons, we won't merge this PR. I understand many will disagree with this decision, but we don't plan to add new formatter options at least until we release Elixir v1.8 (Jan/2019) and the formatter gets more mileage.

@gamache
Copy link
Contributor Author

gamache commented May 16, 2018

Thanks, @josevalim. In that case, I eagerly await the release of 1.7.0 so I can submit this again. 😃

@OvermindDL1
Copy link
Contributor

I also would like to note that the combination of optional parens and keyword lists make trailing commas slightly inconsistent in Elixir as the following code is ambiguous (and therefore disallowed):

Which is exactly why they should not be allowed in parenthesis-less function calls, but should be in multi-line parenthesis function calls, lists, maps, tuples, etc...).

@eksperimental
Copy link
Contributor

For all of those reasons, we won't merge this PR. I understand many will disagree with this decision, but we don't plan to add new formatter options at least until we release Elixir v1.8 (Jan/2019) and the formatter gets more mileage.

Does this mean that this can be discussed/considered before the release of v1.8, or after?

@josevalim
Copy link
Member

It would be after but I would like to reaffirm my personal position on the formatter in general:

  1. I am personally not in favor of global formatter options, especially options that are divergent (i.e. if you toggle the option on and off, every time you get a different formatting)

  2. The direction that I would most likely accept is one that allows trailing commas with some special annotations, there have been some discussions on this area on Could code formatter leave spaces in multiline keyword lists? #6647

  3. I am personally staying away from formatter discussions, so someone should be responsible to build the proposal, collect feedback and push it through. If you need a reference of what a proposal looks like and how to collect feedback from the community, you can see many of the past proposals done by the Elixir team on the Elixir Forum. There is also a great example from @blatyo on the forum. I recommend using the ElixirForum for gathering feedback too but it is up to you. You are welcome to also link to it on the elixir-lang-core mailing list. Once a proposal is written and validated, we will gladly get involved

@eagle-te
Copy link

So....., :trailing_comma option in release v1.10? :p

Personally, I see no reason to not include this. The toggling reason, @josevalim mentioned, could also be applied to line_length option.

There are objectively over weighing good reasons to allow a trailing comma option, such as #6646 (comment) , which shouldn't be dismissed by non productive personal reasoning.

@josevalim
Copy link
Member

josevalim commented Nov 20, 2019

The toggling reason, @josevalim mentioned, could also be applied to line_length option.

Not quite the same. If you have line_length set to 100 and then you set to 80, it will surely introduce line breaks in some cases. But if you roll it back to 100, everything will remain the same.

Trailing commas change everything every time you toggle it.

On the good news, Elixir v1.10 makes public many of the private APIs used by formatter, so now it should be simpler and more stable for third party libraries to provide their own formatting, such as the freedom_formatter from @gamache.

@eagle-te
Copy link

eagle-te commented Nov 20, 2019

Well, I expected at least an answer of compromise, like with :allow_trailing_comma.
Which on true doesn't remove them and add no new ones, but the formatter behaves default, if disabled. This would act non destructive, non toggling, like :line_length, I hinted at.

Thx for the info anyway, probably looking for third party formatters on v1.10 release. :)

@snowe2010
Copy link

Is it time to revisit this? From the arguments I've seen:

Pros:

  1. Trailing commas are good in all locations except parenthesis-less function calls
  2. Trailing commas should not be added by the formatter, just removed. So toggling the option off removes all trailing commas, but turning it on does nothing except allow them.
  3. Trailing commas should only be allowed in multiline situations
  4. Trailing commas have many upsides and few downsides:
    1. Adding a line only changes that line in your VCS diff
    2. Adding a line is much easier. From copy/paste to duplicating a line, no need to add a comma to the end
    3. Sorting the lines is easier

Cons:

  1. Like any formatter option, it allows for something different.
  2. Some people might not understand trailing commas. Personally I think this is a terrible reason, as there are many, many other things beginners wouldn't understand before they got to a trailing comma.

@Sija
Copy link

Sija commented May 2, 2022

Hi! Elixir formatter is a pleasure to work with, except for this one behavior. Could this be revisited, please? Over 4 years has passed since opening this PR and sth like trailing commas are commonly found in virtually every other programming language along with its tooling (linters and/or formatters). Amount of upsides clearly outweigh the few downsides there are.

@marcandre
Copy link
Contributor

@Sija in case it helps: I am maintaining freedom_formatter which supports trailing commas. The new builtin formatter allows plugins and even the default elixir formatter can now be overridden this way. Caveats: I haven't taken the time to setup freedom_formatter to be directly usable as a formatter plugin, it still isn't as easy as if it was builtin, and I haven't checked what level of support packages like ElixirLS / Editor extensions currently offer.

@pmarreck
Copy link

pmarreck commented May 4, 2022

one could get very clever with custom sigil macros that treated unescaped unquoted whitespace as an item separator in lists, keyword lists, maps, etc., the way lisps do etc. (and the way sigils like ~w already work!) without needing to alter the formatter's interpretation of commas

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

Successfully merging this pull request may close these issues.

9 participants