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

Sensible minimum number of char width "won back" before indentation used. #2954

Open
mangelozzi opened this issue Mar 25, 2022 · 9 comments
Open
Labels
F: linebreak How should we split up lines? S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: style What do we want Blackened code to look like?

Comments

@mangelozzi
Copy link

mangelozzi commented Mar 25, 2022

Describe the style change

Black indents items in brackets to help fit more on the screen, however in some cases, one can now see 2 less lines of code, as a tradeoff for seeing 2 more characters. This extra whitespace, along with making the code more verbose could be undesirable. Maybe a sensible default should be chosen that unless the win of pulling the line left is less than x chars, then rather leave it as is. E.g. maybe the minimum win should be say, 4 chars (you guys decide).

Examples in the current Black style

def foo():
    print(
        "This long line is shifted 2 chars (won 2 chars back) to the left due to the indentation, at the expense of adding 2 more lines"
    )

Desired style

def foo():
    print("This long line is not shifted 2 chars to the left due to the indentation, and has 2 less lines of code")

Additional context

Love using Black, it's a game changer thank you!

@mangelozzi mangelozzi added the T: style What do we want Blackened code to look like? label Mar 25, 2022
@felix-hilden felix-hilden added the F: linebreak How should we split up lines? label Mar 25, 2022
@felix-hilden
Copy link
Collaborator

Hi! Tell me if I'm off base here, but isn't that essentially solved by our line length parameter? If we wanted to break only four letters after 88, we could just set the line length to 92. Maybe I misunderstood.

@TomFryers
Copy link
Contributor

TomFryers commented Mar 25, 2022

I think what is meant is that Black should only break lines inside brackets if it results in sufficiently shorter lines. For example, Black currently formats

f(9876543210987654321098765432109876543210987654321098765432109876543210987654321098765432109876543210)

to

f(
    9876543210987654321098765432109876543210987654321098765432109876543210987654321098765432109876543210
)

which results in a longer line. I think the argument is that if this happens (or if breaking lines gives a result that is shorter, but by less than x characters for some small x) then Black should put it onto one line instead.

The current behaviour can be particularly problematic with nested lists. To take an extreme example,

f([[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]])

goes from one line of 89 characters to 87 lines, 41 of which are over 90 characters!

Perhaps it makes sense to be stricter about line lengths near the limit. If it’s 88, then an 88-character line is much more desirable than a 89-character line, but if your line is going to be 200 characters anyway, then is 201 really much of a problem?

@felix-hilden
Copy link
Collaborator

That probably is it, thanks! So I'll respond to that. And to clarify, we're only considering cases where the final formatting still breaks line length limits, right?

To entertain the idea: I agree that it's a bit awkward when a line becomes longer from splitting the line. If we were to have some sort of heuristic for it, I'd argue that it should be simply "does it make the line shorter". So any lines that would with maximal splitting still be equally long or longer should not be formatted. Having any other rule seems arbitrary. And if we agree on that, then only the cases where we have raw bracket pairs or calls whose name is 1-2 chars (+2 brackets <= 4 for the indent) are affected.

But to criticise: ehmm, I honestly didn't come up with examples where this rule would make formatting worse. I think my objections would boil down to adding complexity. The cases presented should be rare, but that isn't a great argument either.

I'm cautiously open to this, but let's hear from other maintainers as well.

@felix-hilden felix-hilden added the S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) label Mar 25, 2022
@JelleZijlstra
Copy link
Collaborator

In the original example the line does actually become shorter, although it's a marginal improvement.

In @TomFryers's example it's true that we make the lines longer, but those examples seem pretty pathological (especially the deeply nested empty list) so I'm not sure it's worth worrying about.

@mangelozzi
Copy link
Author

In the original example the line does actually become shorter, although it's a marginal improvement.

In @TomFryers's example it's true that we make the lines longer, but those examples seem pretty pathological (especially the deeply nested empty list) so I'm not sure it's worth worrying about.

Exactly, there should be a definition of the minimum marginal improvement before using the indentation, e.g. if less than 4 chars improvement, then don't apply the indentation, because its not really adding any value, just making the code verbose and even less readible.

@mangelozzi
Copy link
Author

come up with examples where this rule would make formatting worse. I think my objections would boil down to adding complexity. The cases presented should be rare,

In Django, the _ function is used as a translation function for strings, so one has many strings in the form of:

_("This is the perfect storm for this feature to often make code more verbose")

@mangelozzi
Copy link
Author

f([[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]])

Setting a minmum line width improvement of 4 chars would also solve this example.

@TomFryers
Copy link
Contributor

I had a look for cases of this in some of my code, and I think long string literals are by far the most common cause, and they’ll cease to be a problem when string processing leaves --preview.

@LLyaudet
Copy link
Contributor

I have unfortunately no solution but in real code, I just encountered this problem like this approximately :

if a93:
    return "This is a loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong string"
elif a92:
    return "This is a looooooooooooooooooooooooooooooooooooooooooooooooooooooooooong string"
elif a91:
    return "This is a loooooooooooooooooooooooooooooooooooooooooooooooooooooooooong string"
elif a90:
    return "This is a looooooooooooooooooooooooooooooooooooooooooooooooooooooooong string"
elif a89:
    return "This is a loooooooooooooooooooooooooooooooooooooooooooooooooooooooong string"
elif a88:
    return "This is a looooooooooooooooooooooooooooooooooooooooooooooooooooooong string"
elif a87:
    return "This is a loooooooooooooooooooooooooooooooooooooooooooooooooooooong string"

image

In this context, it is quite unaesthetic apart from the additional lines, it removes uniformity and hence it is less readable.

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? S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

5 participants