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

Keep lists etc split across multiple lines #601

Closed
blueyed opened this issue Nov 9, 2018 · 12 comments
Closed

Keep lists etc split across multiple lines #601

blueyed opened this issue Nov 9, 2018 · 12 comments

Comments

@blueyed
Copy link
Contributor

blueyed commented Nov 9, 2018

Black's wrapping of lists to fit a single line causes diffs to become bigger,
and often makes the code harder to read.

When removing the 3rd line from the list used in the for loop, it will become
short enough to fit a single line and black re-formats it:

Orig:

normid = p.basename + "::TestClass::()::test_method"
for id in [
    p.basename,
    p.basename + "::TestClass",
    p.basename + "::TestClass::()",
    normid,
]:

New:

normid = p.basename + "::TestClass::()::test_method"
for id in [
    p.basename,
    p.basename + "::TestClass",
    normid,
]:

Black:

normid = p.basename + "::TestClass::test_method"
for id in [p.basename, p.basename + "::TestClass", normid]:

I think black should instead keep the original list style (which matches what
black would do with more entries), and only remove the single line.

This would not need an option, but could be respected in general: if a list (or
dictionary etc) is wrapped already like black would wrap it with more entries
than would fit on a single line it should keep it like this.

The list itself should still get re-formatted, but should just be considered to not fit a single line.

@blueyed blueyed changed the title Keep split lists etc Keep lists etc split across multiple lines Nov 9, 2018
@thebigmunch
Copy link

thebigmunch commented Nov 14, 2018

For me this is more about readability as well as consistency.

One case I find it especially taxing to read mixed formatting is when using click decorators. This can require the eyes of a reader to bounce back and forth significantly, making the code more difficult to read.

Another case is comprehensions. I find it is always easier for programmers (especially those new/unfamiliar with Python) to read a comprehensions written like:

[
    expression
    for item in iterable
    if condition
]

I try to write my code so it's as easy as possible for someone to understand and, hopefully, contribute. This is one of the lowest hanging fruits toward that goal. Having this syntax formatted based on line length also creates inconsistency that can confuse people new to Python. Why is it written this way here, but not there?

@ivlevdenis
Copy link

I think need add of the inline parametrize.

@fdemmer
Copy link

fdemmer commented Nov 23, 2018

considering this is on the very top of the documentation:

Black makes code review faster by producing the smallest diffs possible.

i agree.

it also hurts readability, like in this example of a logging configuration:

-        'py.warnings': {
-            'handlers': ['null'],
-            'level': 'WARNING',
-            'propagate': False,
-        },
+        'py.warnings': {'handlers': ['null'], 'level': 'WARNING', 'propagate': False},

@jebob
Copy link

jebob commented Nov 23, 2018

It seems to me that this would break a symmetry. Code formatted with black should be unchanged if we change the line length and then revert the change (black -l 40 then black again).

@JelleZijlstra
Copy link
Collaborator

Yes, as @jebob says, the proposal from this issue would break the larger principle that Black-formatted code looks the same no matter what it originally looked like. Another alternative is to always split collection displays into multiple lines, but that seems undesirable too (we'd split code like x = [1, 2] into four lines). So I don't think we'll change anything here.

@thebigmunch
Copy link

It seems to me that this would break a symmetry. Code formatted with black should be unchanged if we change the line length and then revert the change (black -l 40 then black again).

Yes, as @jebob says, the proposal from this issue would break the larger principle that Black-formatted code looks the same no matter what it originally looked like.

Perhaps I'm missing something... but if this isn't controlled strictly by line length as proposed, it shouldn't be affected by roundtripping changing the line length as described.

Another alternative is to always split collection displays into multiple lines, but that seems undesirable too (we'd split code like x = [1, 2] into four lines)

That is what has been suggested by the OP, myself, and others in this issue as being desirable. Black tries for uniformity, specifically for horizontal whitespace. While I didn't post code, my description of click decorators can easily exhibit a lack of uniformity that is distracting and harder to read. I don't see why changing your given example into 4 lines is undesirable; it would then be consistent with other pieces of code like it within a codebase that gets reformatted onto multiple lines. And arguments given why black splits pieces of code like it still apply (e.g. smaller diffs).

As it is now, I can only possibly use black as a pre-formatter followed by my manually re-formatting the black-formatted code. If it's preferable for the splitting behavior to be controlled by an option rather than hard-coded one way or another, I'm definitely fine with that.

@durin42
Copy link
Contributor

durin42 commented Feb 11, 2019

The way clang-format handles literals of this type is that all elements are kept on a single line iff there's a trailing comma, and if there's no trailing comma then it'll try and pack all on a single line if it can, otherwise do splitting. That has felt like a good compromise to me.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 11, 2019

all elements are kept on a single line iff there's a trailing comma

Wouldn't it make more sense to do so iff there's no trailing comma?
Otherwise mutliline lists wouldn't have it, leading to unnecessary bigger diffs.

@durin42
Copy link
Contributor

durin42 commented Feb 11, 2019

Shoot, I inverted the logic halfway through. Yes, what you said is what clang-format does. Sorry for the confusion.

@zsol
Copy link
Collaborator

zsol commented Feb 14, 2019

We'll keep the current behavior for now.

@vsalvino
Copy link

This issue also bothers me. When defining many lists (or in our case, Django models), it really hurts readability and diffs that some are multi-line and some are single-line.

My thought would be to add a --multiline flag to black that would always split lists, dicts, function calls etc. over multiple lines rather than trying to cram some on a single line and some multi-line. Of course, I can see how this goes against black's philosophy of not providing any configuration options.

Would the project be willing to consider a PR that adds this functionality?

@JelleZijlstra
Copy link
Collaborator

We're hoping the approach in #826 would fix this pain point.

blueyed added a commit to blueyed/pytest that referenced this issue Nov 16, 2019
blueyed added a commit to blueyed/pytest that referenced this issue Nov 16, 2019
vinaycalastry pushed a commit to vinaycalastry/pytest that referenced this issue Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants