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

Finish "magic trailing comma" handling #1288

Closed
ambv opened this issue Mar 3, 2020 · 54 comments · Fixed by #1612
Closed

Finish "magic trailing comma" handling #1288

ambv opened this issue Mar 3, 2020 · 54 comments · Fixed by #1612
Assignees
Labels
T: bug Something isn't working

Comments

@ambv
Copy link
Collaborator

ambv commented Mar 3, 2020

#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.

@ambv ambv added T: bug Something isn't working stable labels Mar 3, 2020
@ambv ambv self-assigned this Mar 3, 2020
@ambv ambv added this to To Do in Stable release via automation Mar 3, 2020
@graingert
Copy link
Contributor

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

@ambv
Copy link
Collaborator Author

ambv commented Mar 3, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator Author

ambv commented Mar 3, 2020

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

@danpalmer
Copy link

@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
Copy link
Collaborator Author

ambv commented Mar 3, 2020

@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
Copy link

@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
Copy link

merwok commented Mar 3, 2020

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

@bersbersbers
Copy link

bersbersbers commented Mar 5, 2020

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
Copy link
Collaborator Author

ambv commented Mar 5, 2020

Yes, it will also apply to method calls.

@bluefish6
Copy link
Contributor

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
Copy link

Misiu commented Mar 17, 2020

Hi all, I have an issue with flake8 and black doing some pre-commit checks.
ref: home-assistant/core#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
Copy link
Collaborator Author

ambv commented Mar 17, 2020

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

@Misiu
Copy link

Misiu commented Mar 17, 2020

Ł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.

@utkarshgupta137
Copy link
Contributor

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,)

Yes, it does. It probably breaks other stuff too and changes the text buffer quite a bit (if you've it set to format on save) which can be jarring.
It would be better to make a keybind for this, and only run it on "compatible" files.

adacore-bot pushed a commit to AdaCore/style_checker that referenced this issue Apr 21, 2021
This commit simply runs "black" on all our Python files except
those in testsuites/tests/.

Note that I ran into one small difficulty when reformatting setup.py,
where black was reformatting a couple of lines in the code in a way
that it triggered the following E231 flake8 violations:

    setup.py:23:69: E231 missing whitespace after ','
    setup.py:25:74: E231 missing whitespace after ','

This is a known issue with black...

    psf/black#1288

... which is fortunately easily worked around by simply removing
the offending comma (black does not add it back).

Change-Id: I492eb1ca5fa371d063e691f7fe06c47daae60d4c
peterjc added a commit to peterjc/biopython that referenced this issue Aug 27, 2021
Used this hack from psf/black#1288

$ pip install black==19.3b0 && black . && pip install black==19.10b && black .

I then manually reverted changes to a handful of explicit data
structures where the magic trailing comma should be retained
(indicates to black not to squash into one line).

Doing this dramatically improves the changes from trying black
version 21.7b0 (right now just four minor changes).
peterjc added a commit to peterjc/biopython that referenced this issue Aug 27, 2021
Used this hack from psf/black#1288

$ pip install black==19.3b0 && black . && pip install black==19.10b && black .

I then manually reverted changes to a handful of explicit data
structures where the magic trailing comma should be retained
(indicates to black not to squash into one line).

Doing this dramatically improves the changes from trying black
version 21.7b0 (right now just four minor changes).
peterjc added a commit to peterjc/biopython that referenced this issue Aug 27, 2021
Used this hack from psf/black#1288

$ pip install black==19.3b0 && black . && pip install black==19.10b && black .

I then manually reverted changes to a handful of explicit data
structures where the magic trailing comma should be retained
(indicates to black not to squash into one line).

Doing this dramatically improves the changes from trying black
version 21.7b0 (right now just four minor changes).
peterjc added a commit to biopython/biopython that referenced this issue Aug 27, 2021
Used this hack from psf/black#1288

$ pip install black==19.3b0 && black . && pip install black==19.10b && black .

I then manually reverted changes to a handful of explicit data
structures where the magic trailing comma should be retained
(indicates to black not to squash into one line).

Doing this dramatically improves the changes from trying black
version 21.7b0 (right now just four minor changes).
@dariocurr
Copy link
Contributor

Is there the possibility to set that the magic final comma must always be used?

I mean, is there a way to say to black that:

l = [1, 2]

must be formatted like this always, and not only if I put the comma?

l = [
    1,
    2,
]

@ggabriel96
Copy link

Is there the possibility to set that the magic final comma must always be used?

I mean, is there a way to say to black that:

l = [1, 2]

must be formatted like this always, and not only if I put the comma?

l = [
    1,
    2,
]

AFAIK not without manually adding a trailing comma: l = [1, 2,]. Then Black will add those line breaks.

@utkarshgupta137
Copy link
Contributor

Is there the possibility to set that the magic final comma must always be used?

I mean, is there a way to say to black that:

l = [1, 2]

must be formatted like this always, and not only if I put the comma?

l = [
    1,
    2,
]

Maybe this hack I shared earlier can be modified for your use case:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.