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

New trailing comma behavior means a list will never be one-line if it ever became multi-line #1742

Closed
itsayellow opened this issue Oct 5, 2020 · 11 comments
Labels
T: bug Something isn't working

Comments

@itsayellow
Copy link
Contributor

itsayellow commented Oct 5, 2020

Describe the bug A clear and concise description of what the bug is.

Apologies if this has been reported, I couldn't find the issue.

Due to the new trailing comma policy, lists that have black run on them as the list expands and contracts can never become one-line after becoming multi-line without user manual editing, because of the trailing comma that black automatically makes after the last element when changing something to multi-line. black interprets this comma as a user directive even though the user never made the trailing comma, black did.

The scenario, in which code is being updated:

  • First, a list with no trailing comma is made long enough that a black re-format makes it multi-line
  • In this case black adds a comma after the last item
  • If enough items for the list are removed, this list will never be collapsed, because black added the trailing comma after the last item

In this case, the user never meant for the list to remain multi-line, the user just wanted black to automatically reformat it as needed. The problem is that black itself automatically added the trailing comma that later was interpreted as something the user added to keep an array multiline. But the user never wanted the array to be multi-line in particular.

Is there a way that black can stay very automatic, so that I don't have to delete the trailing commas of every list to see if it can be collapsed? Currently when a list of items is expanded to multi-line in black it is a one-way operation, that can never be reversed to one-line without user editing that last item.

To Reproduce Steps to reproduce the behavior:

Make a file with these lines:

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

Run Black on it with default arguments.

File becomes:

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

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

Edit the same file by deleting the line "word9", in the file to make the second array shorter.

Run Black again.

Expected behavior A clear and concise description of what you expected to happen.
Since I never originally added a trailing comma myself, I wish for the second array, which now has the same elements as the one-line array above it, to shrink back to one line.

Actually what happens is that Black interprets its own trailing comma as a sign to never shrink the second array to one-line.

Environment (please complete the following information):

  • Version: black, version 20.8b1
  • OS and Python version: [e.g. macOS/Python 3.8.5]

Does this bug also happen on master? To answer this, you have two options:

Yes. (via https://black.now.sh/?version=master)

Additional context Add any other context about the problem here.

@itsayellow itsayellow added the T: bug Something isn't working label Oct 5, 2020
@itsayellow
Copy link
Contributor Author

One solution might be that the "magic trailing comma" could need any comment after it on the same line to be considered "magic". Then it would be sure to be a user-written trailing comma instead of an automatic black trailing comma.

@Spacerat
Copy link

Spacerat commented Oct 28, 2020

I think another possible simple solution would just be for Black to not add the trailing comma when expanding a too-long line across lines. If you add it yourself, it's there to stay.

But the counterargument is this: it's true that if you just delete "word9" and run Black, it puts the comma back. But if you delete "word9" and also the comma after "word8", then run Black, it puts the array back on one line.

Therefore, whenever you modify an list which is spread across many lines, if you want Black to potentially turn it back into a one-liner, just delete the trailing comma, and Black will do the right thing.

@itsayellow
Copy link
Contributor Author

Therefore, whenever you modify an list which is spread across many lines, if you want Black to potentially turn it back into a one-liner, just delete the trailing comma, and Black will do the right thing.

I know that this works (and I mentioned it in my original post). But I don't want to have to go everywhere in my code deleting trailing commas to see if lists can be collapsed. The beauty of black is that it's all automatic (normally) and I don't have to comb through my entire codebase doing manual things like deleting commas just to allow reformatting.

@daveware-nv
Copy link

Just ran into this issue. IMO the entire point of black is that it changes the given code into a canonical form, having a signal that can cause certain behavior is fine. The problem is that black will insert that signal itself which invalidates its use as a user controlled signal.

It's a bad user experience to expect the user to try and delete a trailing comma just in case it might become a single line, only for black to reinsert the comma. Also there are use cases where the user wont even be directly editing code that has been split across multiple lines automatically by black (and hence had a trailing comma inserted automatically), that should now become a single line:

  • using refactoring tools to rename variable/method names to be shorter
  • changing the project maximum line length

@wyfo
Copy link

wyfo commented Apr 2, 2021

The problem is that black will insert that signal itself which invalidates its use as a user controlled signal.

I think you greatly summed up the issue with this sentence.

Still, there is a --skip-magic-trailing-comma/-C parameter (see #1824) to turn off magic comma feature, but not released yet.
Maybe black could add a flag to be used with this parameter to activate the feature in some chosen places, something like:

short_list = [  # fmt: split
    0,
    1,
]

And I rather think it could be the norm instead of the magic comma feature as it is today; of course that's matter of taste, and using -C everywhere is fine too.

By the way this flag could be used as a method chaining flag if it was automatically propagated to chained methods:

obj.method1(  # fmt: split
    some_arg,
).method2(
    other_arg
).method3()

(this would imply a # fmt: split-first to not propagate it if it's not the desired behavior)

@JelleZijlstra
Copy link
Collaborator

This issue simply describes a behavior that was implemented intentionally. As of the last few releases, you can use the --skip-magic-trailing-comma flag to disable the magic trailing comma if you so choose.

@itsayellow
Copy link
Contributor Author

I know it was intentional.

Most of my point is that this feature doesn't work as it was intended: It was intended as a way for the user to indicate to the black formatter how to format text. But because the black formatter itself adds trailing commas, it is not a reliable way to know that the user made the change (and not the black tool itself.)

I have started using that flag, but for a tool that is designed to run on its default settings for most use cases, I think the trailing comma default is not good.

@levsa
Copy link

levsa commented Feb 14, 2022

I too have this problem and in my opinion the flag --skip-magic-trailing-comma does not solve it, since we are using the trailing comma to tell black to keep multi-lines for readability. It would be better if there was a flag to tell black to not add a comma when splitting into multiple lines, but keep the ones there are as they are.

E.g. a flag that, when activated, behaves as so:

  • When there is a trailing comma in the final list element, keep it
  • When splitting a line into multiple lines, do not add a comma after the final element

Edit:

The trailing comma to keep multi-lines even when not necessary is useful to keep multi-lines as in:

x = [
    [
        [0, 0, 0],
        [0, 1, 0],
        [0, 0, 0],
    ],
    [
        [0, 0, 0],
        [0, 1, 0],
        [0, 0, 0],
    ],
    [
        [0, 0, 0],
        [0, 1, 0],
        [0, 0, 0],
    ],
]

@torext
Copy link

torext commented Nov 22, 2023

One solution might be that the "magic trailing comma" could need any comment after it on the same line to be considered "magic". Then it would be sure to be a user-written trailing comma instead of an automatic black trailing comma.

Tihis would seem like a pragmatic option. Could anyone maybe elaborate as to what speaks against this? I sometimes like using the magic trailing comma feature when I'm working actively on something and expecting a collection to grow, but otherwise I much prefer the predictable and vertical-space-saving behaviour of Black using the --skip-magic-trailing-comma option. Having the optionality of indicating the "magicness" of a trailing comma using a comment would allow access to the best of both worlds.

@magnus-bakke
Copy link

E.g. a flag that, when activated, behaves as so:

* When there is a trailing comma in the final list element, keep it

* When splitting a line into multiple lines, do not add a comma after the final element

This would be perfect. We have a higher-than-default line length configured for our pre-commit hooks, but devs still like to do black ., which results in longer lists being expanded with a magic comma, which is irreversible without manually removing commas, and the PR diff gets much larger than it needs to be with no formatting improvements (only more lines).

@JelleZijlstra
Copy link
Collaborator

We have a higher-than-default line length configured for our pre-commit hooks, but devs still like to do black .

If you store the configuration in pyproject.toml, both invocations should use the same configuration. (And you can have a pyproject.toml without having other project-building configuration; at work we have a huge repo with lots of different packages, and the root level has a pyproject.toml with config for Black.)

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
None yet
Development

No branches or pull requests

8 participants