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
Turn off magic trailing commas by default #2135
Comments
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 [tool.black]
skip_magic_trailing_comma = true |
#1742 seems to be solved. What other issues do the trailing commas have? |
@YolCruz for example the issue which is described in the post to which you have just replied. |
@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 |
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? |
There's two separate issues here IMHO.
(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:
|
Imho the easiest solution is to distinguish two scenarios:
Then there's no need for any flag at all, because |
@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. |
@smurfix @jaklan @felix-hilden My two cents is that Black and any code formatters should honor this invariant:
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? |
@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. |
Given Python semantics, line length restrictions, and "magic" comments like those 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
really should be identical to
if at all possible. 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 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. |
While this issue does propose flipping the IMnsHO a better (and less intrusive, all around) way to fix the problem would be to add a new |
I use black quite smoothly lately. Seldom problems. And the trailing comma switch is effective. I like magic trailing commas. |
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). |
That would mean we'd format code like this:
I doubt that's actually what anyone wants. |
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: #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.The text was updated successfully, but these errors were encountered: