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

Black unnecessarily explodes the arguments of the second list of two concatenated lists #1241

Open
hwalinga opened this issue Jan 22, 2020 · 4 comments
Labels
F: linebreak How should we split up lines? T: style What do we want Blackened code to look like?

Comments

@hwalinga
Copy link

I have a few examples with a list with a lot of arguments, where I think especially the second one requires your attention.


alphabet = [
    "Ala", "Arg", "Asn", "Asp", "Cys", "Gln", "Glu", "Gly", "His", "Ile",
    "Leu", "Lys", "Met", "Phe", "Pro", "Ser", "Thr", "Trp", "Tyr", "Val"
]

Becomes

alphabet = [
    "Ala",
    "Arg",
    "Asn",
    "Asp",
    "Cys",
    "Gln",
    "Glu",
    "Gly",
    "His",
    "Ile",
    "Leu",
    "Lys",
    "Met",
    "Phe",
    "Pro",
    "Ser",
    "Thr",
    "Trp",
    "Tyr",
    "Val",
]

This might be more debatable. I think the result is somewhat ugly. Personally, I would black to not touch lines that aren't too long, but I understand that black wants to be a bit more aggressive and I am okay if this keeps the way it is.


alphabet = (
    ["Ala", "Arg", "Asn", "Asp", "Cys", "Gln", "Glu", "Gly", "His", "Ile"] 
    + ["Leu", "Lys", "Met", "Phe", "Pro", "Ser", "Thr", "Trp", "Tyr", "Val"]
)

Becomes

alphabet = ["Ala", "Arg", "Asn", "Asp", "Cys", "Gln", "Glu", "Gly", "His", "Ile"] + [
    "Leu",
    "Lys",
    "Met",
    "Phe",
    "Pro",
    "Ser",
    "Thr",
    "Trp",
    "Tyr",
    "Val",
]

Here the intent of grouping is very clear and the result of black is extremely ugly. Black shouldn't touch this.


alphabet = (
    ["Ala", "Arg", "Asn", "Asp", "Cys"]
    + ["Gln", "Glu", "Gly", "His", "Ile"]
    + ["Leu", "Lys", "Met", "Phe", "Pro"]
    + ["Ser", "Thr", "Trp", "Tyr", "Val"]
)

Doesn't change. I don't know why the previous example got changed and this one did not change.


I think the first example is okayish, and is also not why I raised the issue.

For the second example, I think the grouping made there is explicit enough and shouldn't be touch be black, just like it does with the third example. The third example isn't touched and I expected that to be done for the second as well.

@hwalinga hwalinga added the T: style What do we want Blackened code to look like? label Jan 22, 2020
@JelleZijlstra JelleZijlstra added the F: linebreak How should we split up lines? label May 30, 2021
@felix-hilden
Copy link
Collaborator

This is likely related to #2156, although the case here is for lists. So I'll leave this open.

@pradyunsg
Copy link
Contributor

I would black to not touch lines that aren't too long

Black formats based on the parsed contents of the file. Yes, the output in this case is suboptimal but there's honestly no way to generally format these situations better in general.

There is a mechanism for dealing with situations where a human can do a clearly better job than black at formatting code -- # fmt: off and # fat: on comments.

alphabet = (
["Ala", "Arg", "Asn", "Asp", "Cys", "Gln", "Glu", "Gly", "His", "Ile"]
+ ["Leu", "Lys", "Met", "Phe", "Pro", "Ser", "Thr", "Trp", "Tyr", "Val"]
)

This is basically #2156, and I suggest closing this in favour of that.

@hwalinga
Copy link
Author

@pradyunsg

You are right, this is very much like that other issues, and I went ahead to see if both PR's from the issue would solve this case here.

However, it didn't. Not sure why. I think the list there still triggers some formatting with it, and the implemented change regarding the parenthesis still couldn't prevent that.

However, it seems getting that one PR merged is already a big struggle, so I won't bother them with this edge case.

@boxed
Copy link

boxed commented Jul 17, 2022

Yes, the output in this case is suboptimal but there's honestly no way to generally format these situations better in general.

I would suggest that there is, and that the elm formatter does exactly that. In Elm there are two "styles": vertical and horizontal for many things like lists, enums, dictionaries, function calls, etc. If there is any (user inputted) newline in the line, it will switch from the horizontal to the vertical style.

This rule would make the example work like you'd expect: not trying to join the list to one line. It would also lead to

alphabet = (
["Ala", "Arg", "Asn", "Asp", "Cys", "Gln", "Glu", "Gly", "His", "Ile"]
+ ["Leu", "Lys", "Met", "Phe", "Pro", "Ser", "Thr", "Trp", "Tyr", "Val"]
)

being non-changed (unless it passes the line length rule which also switches to vertical), but it would mean:

alphabet = (
["Ala", "Arg", 
"Asn", "Asp", "Cys", "Gln", "Glu", "Gly", "His", "Ile"]
+ ["Leu", "Lys", "Met", "Phe", "Pro", "Ser", "Thr", "Trp", "Tyr", 
"Val"]
)

would become

alphabet = [
    "Ala",
    "Arg", 
    "Asn",
    "Asp", 
    "Cys", 
    "Gln", 
    "Glu", 
    "Gly", 
    "His", 
    "Ile",
] + [
    "Leu",
    "Lys",
    "Met",
    "Phe",
    "Pro",
    "Ser",
    "Thr",
    "Trp",
    "Tyr",
    "Val",
]

which I would consider much better.

Current black has a way to make the user switch from horizontal to vertical by inserting a trailing comma, but this method doesn't work for python constructs that don't allow trailing comma at all (comprehensions for example) which can be frustrating. This has been the major reason I am against implementing black in iommi right now for example. The newline rule would fix this.

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

No branches or pull requests

5 participants