Finish "magic trailing comma" handling

[ambv]

826 introduced the concept of a magic trailing comma: if you, the programmer, explicitly put a trailing comma in a collection, it signals to Black that you want a given collection exploded into multiple lines, always. While this flies a bit in the way of "doesn't look at pre-existing formatting" and "non-configurability", it is extremely pragmatic and consistent with "make diffs as small as possible".

However, there's issues with it so far. It doesn't yet work on nested collections. Worse yet, in those cases it leaves trailing commas behind when a collection ends up compressed into one line. We need to fix those edge cases to make it generally dependable.

[graingert]

can we have a "black --classic" that ignores trailing commas already in the code?

[ambv]

Do you have a case where this would be useful?

The behavior described here is already the case since the last release. If you remove your trailing comma by hand and re-format, it will behave like you expect.

[graingert]

If you remove your trailing comma by hand and re-format, it will behave like you expect.

I'd like an option where black will remove the trailing commas for me and add them back in if needed

[graingert]

I've taken to running pip install black==19.3b0 && black . && pip install black==19.10b && black . I'd like this wrapped into one command out of the box

[ambv]

If you can implement this as -C (--skip-magic-trailing-comma), I will accept it.

[danpalmer]

@ambv great to see this issue crop up. We've so far avoided using black on the team I work on because we prioritise diff noise (and ease of code review) very highly. Being able to control this is a pragmatic solution to that problem that I think our team will enjoy, and hopefully gets us one step closer to implementing black.

I'm not sure if this is related or a separate issue, but we feel that the 3 forms of expansion (one line, 3-line, and n-line) also contributes to the noise – we only use 2 forms in our style guide (one line, n-line). Do you see this issue as changing that behaviour as well, or do you think that's a separate issue?

---

Expansions being:

# one line
foo = [bar, baz]
def foo(bar, baz): ...

# 3-line
foo = [
    bar, baz
]
def foo(
    bar, baz
): ...

# n-line
foo = [
    bar,
    baz,
]
def foo(
    bar,
    baz,
): ...

[ambv]

@danpalmer, magic trailing commas are a way for you to avoid Black formatting into the 3-line form.

We won't always do this unless you use the trailing comma because it's important for Black to minimize vertical space usage unless you find it important to explode some structure.

[danpalmer]

@Ambv perfect. Sounds sensible. Looking forward to these changes!

We happen to not prioritise vertical space at all, but I realise that’s particular to us.

[merwok]

Could I add: explain how the magic comma work in end-user documentation Thanks!

[bersbersbers]

Since there is quite some variety in the referenced issues, I just wanted to ask if/make sure that this will also apply to method calls. E.g., it seems #826 does not apply to those yet, either:

user@host:~> black --version
black, version 19.10b0
user@host:~> echo "f(1, 2, 3,)" | black - # note: result is on separate line, but not exploded
f(
    1, 2, 3,
)

[ambv]

Yes, it will also apply to method calls.

[bluefish6]

By the way, magical trailing comma works also with multiline imports which would otherwise be collapsed:

This is perfectly acceptable for black 19.10b0

from setuptools import (
    find_packages,
    setup,
)

Which also works perfectly fine with following isort.cfg

...
force_grid_wrap=2
...

(force splitting into multiline import when you import at least 2 things, instead of "do not force splitting" (0))

This produces smaller diffs in case when you change from

from setuptools import (
    find_packages,
    setup,
)

to

from setuptools import (
    find_packages,
    setup,
    xyzxyxz,
)

Maybe you could hint in readme that "A compatible .isort.cfg" with force_grid_wrap=2 is also supported due to the "magical trailing comma"?

[Misiu]

Hi all, I have an issue with flake8 and black doing some pre-commit checks. ref: https://github.com/home-assistant/core/pull/32848 I have this code:

 WEEKDAY_CONDITION_SCHEMA = vol.Schema(
    {vol.Required(CONF_CONDITION): "weekday", "days": weekdays,}
 )

black remove space after last , and then flake8 gives me an error:

E231 missing whitespace after ','

If I add space then black gives me an error, if I remove it then flak8 gives me an error.

I'm new to Python, but looking for a solution to my problem I found this issue. Hopefully, this will be solved. Currently, I have no workarounds and my PR fails.

here are links to Azure pipelines reports: https://dev.azure.com/home-assistant/Core/_build/results?buildId=31820&view=logs&j=2fa2c639-d05e-530d-034e-4fb413fcea4e&t=88bd70b9-15d8-56b4-acb9-94f6fcce0e9d&l=53

https://dev.azure.com/home-assistant/Core/_build/results?buildId=31822&view=logs&j=4e214fed-f310-53fa-0018-b38e7beb405b&t=6344b833-21e0-56d1-3a95-8a0fba7ad97a&l=19

[ambv]

Tomasz, just remove the trailing comma manually for now. Black will not re-add it.

[Misiu]

Łukasz thanks for the help. After removing the trailing comma all my local rules passed. I'll update my PR and hopefully, all the rules will pass there.

[merwok]

Frankly, the style that looks nicest to me and that minimizes future diffs would be this:

WEEKDAY_CONDITION_SCHEMA = vol.Schema({
    vol.Required(CONF_CONDITION): "weekday",
    "days": weekdays,
})

dict on one line with parentheses on other lines is something I don’t remember any human writing.

[Misiu]

@merwok I think the style depends on the content length. This is code formatted by black:

TEMPLATE_CONDITION_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_CONDITION): "template",
        vol.Required(CONF_VALUE_TEMPLATE): template,
    }
)

WEEKDAY_CONDITION_SCHEMA = vol.Schema(
    {vol.Required(CONF_CONDITION): "weekday", "days": weekdays}
)

I personally think the first in much easier to read, but that's how black formatted the code. Ideally, this should be styled in the same way, like this:

TEMPLATE_CONDITION_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_CONDITION): "template",
        vol.Required(CONF_VALUE_TEMPLATE): template,
    }
)

WEEKDAY_CONDITION_SCHEMA = vol.Schema(
    {
        vol.Required(CONF_CONDITION): "weekday",
        "days": weekdays,
    }
)

@ambv what do you think?

this is the precommit config:

  - repo: https://github.com/psf/black
    rev: 19.10b0
    hooks:
      - id: black
        args:
          - --safe
          - --quiet
        files: ^((homeassistant|script|tests)/.+)?[^/]+\.py$

[thnee]

For what it's worth, I would definitely prefer that black just lets us choose, rather than force single line or multiple lines, and then try to detect it based on magic commas.

In general, it's good that black is strict and forces some things, but not in in this case. Black does not need to force code into one line at all, that is just overzealous. This is literally my one and only gripe with black.

This is exactly what we discussed here: https://github.com/psf/black/issues/1054#issuecomment-549368939

But never heard anything back in that issue.

[aiguofer]

I'm on the same camp as @thnee. It's not uncommon for my lists, params, etc to grow until they expand from one line to many. Worse yet, I've also had to remove something which then brings it back to a single line. Each of those commits is quite ugly, and it also makes the flow of the code a little awkward.

I'd love to have the option to always expand (with the only exception maybe being when there's a single item, but if that's too hard to implement I'm ok with the expansion even in this case).

[RickyDepop]

I'm on the same page as @aiguofer and @thnee

[thnee]

I would also prefer that black removes trailing commas if the code is on a single line.

So this should not be modified, because there is absolutely nothing wrong here, it's good code.

x = [
    "a",
    "b",
]

But this

x = ["a", "b",]

Should be changed to this

x = ["a", "b"]

Again, this should apply the same way to all similar things, such as: lists, dicts, imports, declaring functions and classes, calling callables, etc.

[zyv]

@ambv awesome idea, we'd love to see magic trailing commas supported on nested collections!

It seems to be the only stumbling block that currently prevents the migrations of all our projects to black, especially because of the annoying interaction with flake8, but also because nested collections (dicts) when subdicts become unreadable when they are inconsistently collapsed to one line and there is currently no way to prevent it from happening...

[jdmoorman]

Support for nested collections will make black much more appealing for academic projects where matrices are frequently hard coded. e.g.

# fmt: off
A = np.array([
    [1, 0, 0],
    [0, 1, 0],
    [0, 0, 1]
])
# fmt: on

could be written as

A = np.array([
    [1, 0, 0],
    [0, 1, 0],
    [0, 0, 1],
])

This has been a stumbling block when introducing my collaborators to using black. I look forward to its resolution.

[WhyNotHugo]

Is there a workaround until this is in place? Because of #1409, black creates results that make flake8 fail.

[bhrutledge]

In an effort to understand the current behavior, I put together some examples using the Black Playground.

Here's a preview:

[WhyNotHugo]

I'd say line 29 is correct, and that the func args is the bug.

The whole point of the magic comma is for it to render like that.

[freakboy3742]

@WhyNotHugo To confirm: are you're saying magic_dict_one_line (line 29 on output) is the bug, or magic_nested_dict (line 29 on input)?

[bhrutledge]

@freakboy3742 I read that as magic_dict_one_line (line 29 on output) is correct, and magic_args (line 51 on output) and/or magic_args_one_line (line 58 on output) is the the bug.

[WhyNotHugo]

I meant to say magic_dict_one_line (line 29 on output).

[bhrutledge]

warning: possible invocation of Cunningham's law

One trick I found to mitigate this is to run add-trailing-comma before and/or after black. In addition to adding commas that will trigger the "magic" multi-line behavior, it will also remove superfluous commas like foo(kwarg='bar',) or {"d": 4, "e": 5, "f": 6,}.

Caveat: on a legacy codebase, it took me a few iterations of running add-trailing-commas src/**/*.py then black src to get to a stable result, where neither tool made further changes.

[a1tus]

One trick I found to mitigate this is to run add-trailing-comma before and/or after black.

This issue is the only one that stops us from using black in our project and I'm so glad to see that someone else was trying to resolve it with exactly the same kludge. Consequent runs really do the trick but still waiting for native solution.

[haizaar]

I'm a bit confused - this feature is expected to work in 19.10b0, but it doesn't for the following snipped:

$ black --diff setup.py 
--- setup.py    2020-07-04 15:39:52.554322 +0000
+++ setup.py    2020-07-04 15:40:25.772812 +0000
@@ -3,16 +3,9 @@
 setup(
     name="http-noah",
     version="0.0.0",
     url="https://github.com/haizaar/http-noah",
     packages=find_packages(),
-    install_requires=[
-        "structlog",
-        "pydantic",
-    ],
-    extras_require={
-        "async": ["aiohttp"],
-        "sync": ["requests"],
-        "all": ["aiohttp", "requests"],
-    },
+    install_requires=["structlog", "pydantic",],
+    extras_require={"async": ["aiohttp"], "sync": ["requests"], "all": ["aiohttp", "requests"],},
 )

reformatted setup.py
All done! ✨ 🍰 ✨
1 file reformatted.

As you can see, arguments for install_requires and extras-require are still being imploded despite trailing commands. Should I open a separate bug or is it by design?

[ichard26]

@haizaar It currently doesn't work on nested collections. This is mentioned in the first issue comment.

However, there's issues with it so far. It doesn't yet work on nested collections. Worse yet, in those cases it leaves trailing commas behind when a collection ends up compressed into one line. We need to fix those edge cases to make it generally dependable.

A new issue is probably not necessary. This one is probably enough -> #1377.

[haizaar]

@ichard26 Thanks for the clarification. Subscribed to #1377.

[auscompgeek]

@haizaar FWIW, for the particular case of a simple setuptools setup.py, you could instead use setup.cfg files.

[marctorsoc]

Yes, it will also apply to method calls.

The main showstopper for my team to adopt black is the lack of magic comma for method calls. What's the state of this @ambv ? I'm happy to collaborate but will need some guidance

[chebee7i]

Is there active development happening on this issue?

There are a number of issues that are negatively impacting the use of black in production code, but it seems like active development work has sort of stalled again. Life is busy, I understand. If resources are presently scarce, is there a way that the community could vote on which issues should take top priority? Are the reactions to each issue consulted when choosing what to work on?

Fully support (which means also handling #1169 and #1433) for the magic trailing comma is pretty high on my list.

[dwaxe]

is there a way that the community could vote on which issues should take top priority?

Github recently added the ability to sort issues by reaction counts. This could serve as a decent proxy for community votes: https://github.com/psf/black/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc

[GianlucaFicarelli]

Thanks for fixing this issue. I tried and it's working in the usual cases, but I found a problem in this specific case:

    x = {
        "a": 1,
        "b": 2,
    }["a"]

using black (master, commit cd3a93a1) it's reformatted to

    x = {"a": 1, "b": 2,}[
        "a"
    ]

while I was expecting no reformatting.

[ambv]

Very good catch, @GianlucaFicarelli. This is a bug due to there being a chain of bracket pairs to explode and Black explodes the wrong one. It's relatively minor fortunately but I won't have time to deal with it before today's release. We'll fix it in the next round. I opened a separate bug for this.

[marcglobality]

@ambv is there any release plan in black? i.e. what things get into each release, when are they planned to happen, etc

[osandov]

I think this change is reasonable, but it has the unfortunate side effect that long lists that get split into multiple lines will often get "stuck" that way even if the list gets smaller.

For example, say I write this code:

class Foo:
    def bar(self):
        if baz:
            some_long_contrived_function_name(A_LONG_ARGUMENT, ANOTHER_LONG_ARGUMENT, AN_EXTRA_LONG_ARGUMENT_TO_OVERFLOW_COLUMNS)

Black will rewrite it to:

class Foo:
    def bar(self):
        if baz:
            some_long_contrived_function_name(
                A_LONG_ARGUMENT,
                ANOTHER_LONG_ARGUMENT,
                AN_EXTRA_LONG_ARGUMENT_TO_OVERFLOW_COLUMNS,
            )

But then let's say I refactor to remove the last argument. I'd delete the last line, resulting in this:

class Foo:
    def bar(self):
        if baz:
            some_long_contrived_function_name(
                A_LONG_ARGUMENT,
                ANOTHER_LONG_ARGUMENT,
            )

And Black would leave that alone. But I'd like for it to rewrap it to this:

class Foo:
    def bar(self):
        if baz:
            some_long_contrived_function_name(A_LONG_ARGUMENT, ANOTHER_LONG_ARGUMENT)

If I notice this, I can of course remove the last comma and rerun Black, but the nice thing about Black is that I don't have to think about these things :) are there any thoughts about how to handle this? Perhaps the hypothetical --skip-magic-trailing-comma option you mentioned?

[marctorsoc]

I don't really see a problem @osandov . If you write the magic comma you want it exploded. Otherwise you leave black to do its thing

[osandov]

@marctorsoc but I didn't write the magic comma, Black put it in when it exploded it the first time (obviously there's no way to distinguish that).

[marctorsoc]

oh I see, sorry misunderstood. Then maybe the problem is black putting the magic comma in the first place then...

[PeterJCLaw]

I think this change is reasonable, but it has the unfortunate side effect that long lists that get split into multiple lines will often get "stuck" that way even if the list gets smaller.

While from one perspective this can appear unfortunate, for some (myself included), this behaviour is a desirable effect of the magic trailing comma handling. Note that a result of this behaviour is that it prevents black from introducing diff noise associated with reformatting the list onto a single line unless the code author explicitly asks black to do so.

[flying-sheep]

Exactly, it’s amazing! Thanks to the authors for making black better even if that means some build breakage here and there. A feature like this is exactly what I expect to get in return when that happens in beta or a major version change.

[graingert]

skip-magic-trailing-comma looks like it would be easy to do in a third party program that you configure to run before black, eg modify the source code from flake8-commas to strip all trailing commas before passing the source to black

@osandov @tyleryep if you do build a tool that does this - please ping me (keybase is best) as I'd be interested to see it

[utkarshgupta137]

atom.workspace.observeTextEditors (editor) ->
    editor.buffer.onWillSave ->
        if (editor.getPath().endsWith('.py'))
            editor.setText(editor.getText().replace(/,\s*\)/gm, ")"))
            editor.setText(editor.getText().replace(/,\s*\}/gm, "}"))

For those who want minimum trailing commas, you can add this to .atom/init.coffee. It will remove all trailing commas from python files before saving, and then black will add them again as required.

[TylerYep]

atom.workspace.observeTextEditors (editor) ->
    editor.buffer.onWillSave ->
        if (editor.getPath().endsWith('.py'))
            editor.setText(editor.getText().replace(/,\s*\)/gm, ")"))
            editor.setText(editor.getText().replace(/,\s*\}/gm, "}"))

For those who want minimum trailing commas, you can add this to .atom/init.coffee. It will remove all trailing commas from python files before saving, and then black will add them again as required.

Doesn't this break one-element tuples? e.g. x = (5,)