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

Different style depending on trailing comma #1643

Closed
erdnaxeli opened this issue Aug 27, 2020 · 6 comments
Closed

Different style depending on trailing comma #1643

erdnaxeli opened this issue Aug 27, 2020 · 6 comments
Labels
T: style What do we want Blackened code to look like?

Comments

@erdnaxeli
Copy link

erdnaxeli commented Aug 27, 2020

Hi,

First I would say I really love black, it is an awesome project that I have being using for years now.

I get that the trailing comma is a long standing bug that is now fixed, but I am somewhat not happy with the fix.
Black now does this :

  • [1,2,3] stay as is
  • but [1,2,3,] becomes
[
    1,
    2,
    3,
]

Previously the second example would have stay on one line.

This bothers me. A comma is not significant here, and as black removes useless parentheses or spaces, I would expect it removes the useless comma and keep my list on one line.

Now the style will be different if the dev added a useless comma or not.

More pernicious, look at this example :

  • step 1
a = [11111111111111111111111111, 22222222222222222222222222222, 33333333333333333333333, 444444444444444444444]
  • step 2: I run black on it
a = [
    11111111111111111111111111,
    22222222222222222222222222222,
    33333333333333333333333,
    444444444444444444444,
]
  • step 3: I changed my mind, let's remove those 3s and 4s
a = [
    11111111111111111111111111,
    22222222222222222222222222222,
]
  • step 4 : I run black, and … it does nothing

Actually to synthesize what bothers me is that black does nothing when I have this

a = [
    11111111111111111111111111,
    22222222222222222222222222222,
]

a = [11111111111111111111111111, 22222222222222222222222222222]

Those two lists are semantically identical, I would expect black to indent them the same way.

I get this is related to #826 and #1288, and this is a feature. But I don't really see the point. As black can add commas, I would expect it to remove commas to.

Thanks.

@erdnaxeli erdnaxeli added the T: style What do we want Blackened code to look like? label Aug 27, 2020
@erdnaxeli
Copy link
Author

erdnaxeli commented Aug 27, 2020

Just two more thoughts about tihs:

  • the README says "you agree to cede control over minutiae of hand-formatting", so I don't see why we want to allow to force expansion
  • now there will be discussion in PRs about trailing comma or not, those discussions that black what supposed to avoid.

@ambv
Copy link
Collaborator

ambv commented Aug 27, 2020

This feature has been there since 19.10b0 and documented: https://black.readthedocs.io/en/stable/the_black_code_style.html#the-magic-trailing-comma

It just used to work rather unpredictably which was fixed by 20.8b0.

The rationale is to decrease diff size. If you want to prepare your code for a growing list, dictionary, etc., you use a trailing comma and it keeps the bracket pair exploded with one item per line.

@ambv ambv closed this as completed Aug 27, 2020
@tstraley
Copy link

tstraley commented Sep 3, 2020

@ambv In this section of the style guide it states something different https://black.readthedocs.io/en/stable/the_black_code_style.html#trailing-commas

Black will add trailing commas to expressions that are split by comma where each element is on its own line. This includes function signatures.

Unnecessary trailing commas are removed if an expression fits in one line.

It seems like black 20 is now very explicitly NOT removing unnecessary trailing commas, and instead forces them onto multiple lines (when unnecessary) and it is on the user to adjust from black 19 and remove these trailing commas to keep on a single line?

Additionally, the line wrap section of the doc https://black.readthedocs.io/en/stable/the_black_code_style.html#how-black-wraps-lines
includes an example that explicitly tries to move things onto a single line when it fits. This is now tied to whether the user included a trailing comma or not, which is not documented and seems incorrect.

I'd like to request this get reopened based on the discrepancy with this documentation.

@ichard26
Copy link
Collaborator

ichard26 commented Sep 3, 2020

Hey @tstraley you appear to be a bit misinformed. You are reading an old version of the documentation which is outdated/incorrect in a few ways. If you look at the latest version (i.e. on the master branch), those now incorrect paragraphs been replaced with a paragraph linking to the also new section about the "magic trailing comma" feature

edit: the new section detailing the "magic trailing comma" feature is present in the stable version of the docs, while the incorrect paragraphs were removed after the new stable docs were released

@tstraley
Copy link

tstraley commented Sep 3, 2020

@ichard26 thanks for pointing me to the master docs yet to be published. Is there a change to fix the wrap-lines section that suggests black should be collapsing things down to one line when it fits? Thanks!

@ichard26
Copy link
Collaborator

ichard26 commented Sep 3, 2020

That section isn't wrong or incorrect per se, but it is definitely is missing some finer points. Black would still collapse examples like these when it doesn't violate the line length limit and there is no trailing comma:

this_is_a_tuple = (
    1,
    2,
    3
)

IMO what the section needs is an note that points to the "magic trailing comma" as it does go against "Black ignores previous formatting" (which is generally true, but not in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

4 participants