Unnecessary line breaks in method call on multiline string

[lubomir]

This is possibly not a bug, but the results are unexpected and don't really seem to improve readability. However feel free to close if this is actually correct.

Operating system: Fedora 28
Python version: 3.6.5
Black version: 18.5b0
Does also happen on master: yes

Formatting this file uses some extra unexpected new lines:

MULTILINE = """
foo
""".replace("\n", "")

@@ -1,4 +1,6 @@
 MULTILINE = """
 foo
-""".replace("\n", "")
+""".replace(
+    "\n", ""
+)

[ambv]

Black considers multiline strings as always not fitting a single line (per definition), so it tries to break the line if possible. I agree that in this case the result is sub-optimal. I'll see what I can do to improve this.

[zsol]

@AvdN points out another effect of how black treats multiline strings that are styled like this in #426 (in the case when the string is a parameter to a function call)

[ambv]

A related case is:

textwrap.dedent("""\
    Hello, I am
    a multiline string used with
    a common idiom
""")

We need to fix that, too.

[OJFord]

That related case is how I got here. For clarity here, the above:

textwrap.dedent("""\
    Hello, I am
    a multiline string used with
    a common idiom
""")

becomes:

textwrap.dedent(
    """\
    Hello, I am
    a multiline string used with
    a common idiom
"""
)

Amusingly you can of course instead change it to:

s = """\
    Hello, I am
    a multiline string used with
    a common idiom
"""
s = textwrap.dedent(s)

in one fewer lines and which black will accept (and won't churn when this is resolved) - or do the dedent call at the point of use.

[brupelo]

What's the status of this one? I've seen this one has been opened around 2018 and evolved quite a bit over the years... if I'm not mistaken it seems black is still screwing up this common & readable pattern:

if __name__ == "__main__":
    transform(textwrap.dedent("""
        # from random import random

        def random():
            return 10

        a = random()*10
    """))

into something like this:

if __name__ == "__main__":
    transform(
        textwrap.dedent(
            """
        # from random import random

        def random():
            return 10

        a = random()*10
    """
        )
    )

One of the main reasons to use textwrap.dedent pattern with multiline strings is because "indentation awareness"... But in this case, that sense of indentation will be lost thanks to black... said otherwise, in this particular case black has destroyed the code into something much worse.

Black version: black, version 19.10b0
Python: 3.6.8 (tags/v3.6.8:3c6b436a57, Dec 23 2018, 23:31:17) [MSC v.1916 32 bit (Intel)] on win32

[brupelo]

I love black... I really do... but this issue is really nasty... not just because black will breaks code folding completely but also because the output produced by black doesn't make any sense in comparison the original clean version. I find myself adding clauses # fmt: off & # fmt: on more than I should when that shouldn't be case.

I understand none of the maintainers is interested on this issue? I've see both this one and other related ones have been opened for years... question for people who've submitted patches to black before, do you what's the place in the codebase where this is handled?

Thanks.

Ps. Why this important issue hasn't been fixed already? Has it been considered a non-go? Too difficult to handle? I'd like to understand... :)

[FlipperPA]

This is a big pet peeve for me too. There's information about contributing here, if you're interested, @brupelo:

https://github.com/psf/black/blob/master/CONTRIBUTING.md

[FlipperPA]

Some quick questions:

• would the preferred behavior be to leave the original code intact?
• would the preferred behavior be to format all docstrings within parentheses the shorter, more beautiful way?

I'm trying to get a sense if this issue clears the hurdle mentioned in the contribution README: On the other hand, if your answer is "because I don't like a particular formatting" then you're not ready to embrace Black yet. Such changes are unlikely to get accepted. You can still try but prepare to be disappointed.

I'd be happy to take a stab if someone would be willing to do some hand holding along the way, as I'd be a brand new contributor.

Thanks for your efforts, black is awesome and amazing. 💯

[merwok]

The point about «liking» is dismissive of more serious concerns. Single vs double quotes is an annoying change but mostly irrelevant (it was nice to match the style that the Python REPL outputs, but quote style doesn’t matter and there is a point that it’s common to have apostrophe in a string, so 🤷). But people care about diffs and history, and the developer is the person best situated to decide if this is an extensible list/dict/set that is multiline from the start, or something that can be rewritten by a style program. (One of the worst cases is parameters to the pytest.parametrize decorator that are rewritten to waste screen space without benefit.)

[aneeshusa]

I kept running into this so I decided to spend some time figuring how to fix it. Happy to say I have a branch that should do just that! It's got a new 316 line test file with a bunch of cases (including the ones reported in this and linked issues). However, the code is still messy as I haven't yet taken the time to clean it up: right now it's ~100 new lines dumped into the is_line_short_enough function. I'll post a draft PR in the next day or two after some basic cleanup, just wanted to comment to let others know if they had been thinking of spending time on this.

FWIW, the branch is more of a POC right now and requires more work before it could be mergeable. I have some hacked-together code for bracket tracking, trailing comma handling, etc., which will require some refactoring so I can reuse the existing functions for that (will need help from the black folks on the best approach). There's also more work to do like finishing up draft changes to the black style guide docs & CHANGES.rst looking through the black-primer output to verify the changes and work with the relevant upstreams.

[aneeshusa]

Opened #1879 with my prototype fix! Would love feedback from folks on the new tests I added and whether the formatting I chose makes sense (AFAIK I included all the examples from this and linked issues).

[arogozhnikov]

Just want to bring this up if it is forgotten.

I think it's the only annoying (and undoubtedly annoying) part of black formatting to me,
specifically examples provided by @ambv and @OJFord : #256 (comment)

Maybe a heuristic like this would work: combinations like (""" and """) shouldn't be split by formatting (if they stick, just don't split them).

[aneeshusa]

Some context from me:

• I'm planning to roll out black across all repos at my company (I'm on the languages team at Lyft). Similar to you, this is the only piece of black formatting that I've found annoying enough to consider a blocker since we have a number of repos that use multi-line strings heavily.
• I haven't forgotten about the PR, but unfortunately it has been put on the backburner due to other internal priorities.

I would like to get back to the PR later this year but can't give any guarantees as to when that'll be. In the meantime, I welcome anyone to take over the work and make their own PR!

[ichard26]

Worst case scenario is that I take over the PR and try to push it to completion but unfortunately I've been extremely busy lately too so it'll have to wait at least more week before I have time (and even then the core team is busy figuring out the stable release).

[FlipperPA]

@ichard26 @aneeshusa Any idea how much more code work the PR needs? Or is it more of someone to shepherd it across the finish line?

[aneeshusa]

#1879 (comment) is still pretty accurate. Here's my take on action items, though I haven't looked at this code or black internals since I opened that PR.

• Looks like there is general consensus on the new formatting style, so checkboxes 1 & 2 are done.
• The code needs to be moved around a bit now that black's source code has been split into multiple files and isn't one giant file - I suspect this would be pretty straightforward to do.
• The current PR adds a bunch of extra work into is_line_short_enough, though it's a no-op when not looking at a MLS-containing line. I suspect it'll need to be reworked to use BracketTracker for speed (or at the very least pick up some of the bugfixes it has https://github.com/psf/black/pull/1879/files#diff-d23a6917d7262ca9815093bffb7148299e516575251959ed9b405b648e76aa29R6480 for correctness), but I hadn't gotten far enough to understand what exactly this could/would look like. It's also possible that refactor is is an optimization we could make later - some profiling data would confirm how this affects the performance. This is the biggest open AI as I see it. Advice from maintainers would be very helpful here as well (this is checkboxes 3 and 4).
• Checkbox 5 - adding docs - might require a few iterations to make it clear but should be straightforward.

If, based on profiling and maintainer feedback, the current approach in the PR is mostly acceptable, then it's more of a "take it across the finish". If the approach needs some rework then it'll require more investment depending on what changes are needed.

[JelleZijlstra]

Also, with the new stability policy, this change would need to go only into the --preview style.

[aneeshusa]

Quick update: @olivia-hong who works with me on the Backend Language Tooling team at Lyft is going to be picking this work up again!

[FlipperPA]

Thank you @aneeshusa and @olivia-hong! Lyft was the go-to ride of choice for us in San Diego at DjangoCon US a few weeks ago. 😀

[olivia-hong]

Posting an update here for visibility as well: I've updated the PR Aneesh originally put up and it's now fully ready for review #1879 (comment)! Comments/feedback are much appreciated.

Since the issue is a few years old at this point, tagging some of the original reviewers @JelleZijlstra @ichard26 as an FYI. Apologies for any noise.