Turn off magic trailing commas by default

[jaklan]

Is your feature request related to a problem? Please describe. Since magic trailing commas were introduced, they became the source of many unexpected behaviours: https://github.com/psf/black/search?q=magic+comma&type=issues

Especially painful are cases like this one: https://github.com/psf/black/issues/1742, which cause Black to force usage of magic trailing commas even if that wasn't an intention of a developer.

Black itself is still considered as beta, but then magic trailing commas seems more like alpha. Before their appearance, Black's approach was to generate either a) most concise (one-line) or b) most readable (multi-line) code - in that order. Now - it's not more the case, because we can easily end up with multi-line code, which could be fitted into one line, but it's blocked by automatically generated comma. To avoid it, you have to manually delete trailing comma - which means you have to actually think about the formatter and know its specificity, where the whole idea about Black was don't configure, just run and forget.

Describe the solution you'd like

Turn off magic trailing commas by default and change the --skip-magic-trailing-comma flag to --enable-magic-trailing-comma for people who are actually aware of current flaws, but consciously accept them. At least until most of the major gotchas are resolved.

[hrldcpr]

Well said! This has been my main frustration with Black ever since I started using it—once a list has become long enough for Black to make it multi-line, it will never return to one line even if you make the list shorter?!

I thought it was just a bug until I finally found the ominous term "magic trailing comma". I expect I'm not alone! (And I still think of it as a bug, honestly…)

In the meantime, in case it's useful to anyone else, you can globally fix this by creating the file ~/.config/black with the contents:

[tool.black]
skip_magic_trailing_comma = true

[YolCruz]

#1742 seems to be solved. What other issues do the trailing commas have?

[jaklan]

@YolCruz for example the issue which is described in the post to which you have just replied.

[YolCruz]

@jaklan you're just complaining that the setting is on by default but you're not really giving a concise example of an issue that they have

[hrldcpr]

@YolCruz as far as I can tell, #1742 wasn't "solved", it was just closed as intended behavior. (And in my opinion the intended behavior is incorrect.)

[jaklan]

@YolCruz issue #1742 which is linked the above post is describing the problem in details. Have you actually verified why was it "solved" or just looked at the issue label?

[felix-hilden]

I'd be willing to entertain us flipping the toggle in preview and eventually altogether in v23. But this is just me personally 😅 Not collapsing small collections is more irritating to me than slightly larger diffs, although the config option is an easy fix. This would get us a step closer to our whole "not taking previous formatting into account" philosophy. Thoughts by other maintainers?

[smurfix]

There's two separate issues here IMHO.

1. honor the trailing comma as a way to signal Black not to re-wrap a list as a single line.
2. don't add a trailing comma if there was no such thing before reformatting.

(1) is disabled by the skip-magic-trailing-comma option, or at least that's what the documentation says. Personally I don't particularly care about this option.

However, I don't think there's a good reason for (2) to exist in the first place. Reasons:

• it subtly changes the original source instead of merely reformatting it
• changing line lengths from A to B and back should be idempotent: black -l 100 foo.py should result in the same file as black -l 50 foo.py && black -l 100 foo.py.

[jaklan]

Imho the easiest solution is to distinguish two scenarios:

1. No trailing comma added by developer

   ["word1", "word2", "word3", "word4", "word5", "word6", "word7", "word8", "word9", "word10"]
   

   becomes:

   [
       "word1",
       "word2",
       "word3",
       "word4",
       "word5",
       "word6",
       "word7",
       "word8",
       "word9",
       "word10"
   ]
   

   and when you remove some words to fit into max line length:

   [
       "word1",
       "word2",
       "word3",
       "word4"
   ]
   

   it again becomes:

   ["word1", "word2", "word3", "word4"]
   

2. Trailing comma added by developer

   ["word1", "word2", "word3", "word4",]
   

   always becomes:

   [
       "word1",
       "word2",
       "word3",
       "word4",
   ]
   

   no matter how long the list is and it's never wrapped into one line if you don't remove the trailing comma explicitly.

Then there's no need for any flag at all, because black would never add / delete any trailing comma automagically. We would just have to accept the fact that the lack of trailing comma is fully acceptable - the current problem exists because black adds it whenever possible, so we actually try to solve a problem which is caused by some troublesome assumption. If we change the assumption - we get rid of the problem as well.

[felix-hilden]

@smurfix and @jaklan, it's unlikely that we'll change our stance on trailing commas in general because of the consistency and diff reduction. And making Black consider if there was a trailing comma or not is exactly the dependency to previous formatting we'd want to reduce if we were to disable the magic comma by default.

[AndreaCensi]

@smurfix @jaklan @felix-hilden My two cents is that Black and any code formatters should honor this invariant:

The input and output files have exactly the same sequence of non-whitespace characters, and evaluate to the same AST.

That is, a "code formatter" should only change the whitespace characters in a file while, of course, maintaining the same semantics.

This invariant is what I think people in the discussion are after, abstracting away from the particular case of the comma.

(How to check the invariant? Simply remove all whitespace characters from the two files and check that they are the same (plus, check that they give the same AST.)

For the particular case of the comma, I personally like the other rules around it (comma at the end → distribute across lines; if not, try to fit). What seems weird to me is that black decides to introduce a comma where there wasn't one, thus overriding the will of the developer.

--

@felix-hilden If one wanted to modify Black's source code oneself to obtain a behavior that respects the invariant above, would you anticipate that to be an easy job?

[JelleZijlstra]

@AndreaCensi Black already checks that it preserves the AST (with a few minor exceptions, mostly around docstring formatting). We do not aim to preserve the sequence of non-whitespace characters, and I don't think that's something we should attempt to do. We may, for example, add or remove parentheses; add trailing commas; change quotation styles; change the case of numeric literals and string prefixes.

[smurfix]

That is, a "code formatter" should only change the whitespace characters in a file while, of course, maintaining the same semantics.

Given Python semantics, line length restrictions, and "magic" comments like those pylint interprets, this is not possible in general.

IMHO a better invariant is that calling Black with settings A always results in output X, no matter which other settings B it has been called with before that. Thus the result of

black --line-length 99 foo.py

really should be identical to

black --line-length 49 foo.py
black --line-length 99 foo.py

if at all possible.

Auto-adding trailing commas means that this invariant cannot hold.

[JelleZijlstra]

Auto-adding trailing commas means that this invariant cannot hold.

That's only true as long as we don't also auto-remove trailing commas when appropriate. We do that already in --skip-magic-trailing-comma mode; this issue proposes making that behavior the default.

There are other areas (around parentheses) where the invariant you propose doesn't currently hold, but I agree it's a good goal to work towards.

[smurfix]

this issue proposes making that behavior the default.

While this issue does propose flipping the --skip-magic-trailing-comma flag's default, flipping its default would be quite disruptive and I for one definitely wouldn't want that.

IMnsHO a better (and less intrusive, all around) way to fix the problem would be to add a new --add-trailing-comma option that controls adding, not reacting to, trailing commas. We can then bikeshed what that option's default should be 😁

[drunkwcodes]

I use black quite smoothly lately. Seldom problems. And the trailing comma switch is effective. This feature is still not significant at the first glance, and should be promoted more.

I like magic trailing commas.

[mcobzarenco]

I wish there was a setting to always insert "magic commas" -- so formatting is consistent and deterministic (and you don't have to litter reviews reminding people to add a magic comma).

[JelleZijlstra]

I wish there was a setting to always insert "magic commas"

That would mean we'd format code like this:

% black -c 'for x in range(len(lst,)): pass'
for x in range(
    len(
        lst,
    )
):
    pass

I doubt that's actually what anyone wants.