multiline_string_handling sometimes leads to worse formatting

@amyreese

Reports from @amyreese in #4042 (comment):

# before 
Thing(
    """
    value
    """
)

# after
Thing("""
    value
    """)

# before
VALUE = [
    Thing(
        """
        value
        """
    )
]

# after
VALUE = [Thing("""
    value
    """)]

# before
func(
    (
        value
        + """
        something
        """
    ),
    argument,
)

# after
func(
    (value + """
        something
        """),
    argument,
)

# before
def foo():
    result = (
        """
        something
        """
        + value
    )

# after
def foo():
    result = """
        something
        """ + value

I just enabled only multiline_string_handling and ran Black on my work codebase. Many changes are improvements, but quite a few look uglier.

Some examples:

         xxxx_xxxxxxx.xxxxxx_xxxxxxxxxxxx_xxxxxx_xx_xxx_xxxxxx: {
             "xxxxx": """Sxxxxx xxxxxxxxxxxx xxx xxxxx (xxxxxx xxx xxxxxxx)""",
-            "xxxxxxxx": (
-                """Sxxxxxxx xxxxxxxx, xxxxxxx xx xxxxxxxxx 
-                xxxxxxxxxxxxx xxxxxxx xxxxxxxxx xxx-xxxxxxxxxx xxxxxx xx xxx-xxxxxx"""
-            ),
+            "xxxxxxxx": ("""Sxxxxxxx xxxxxxxx, xxxxxxx xx xxxxxxxxx 
+                xxxxxxxxxxxxx xxxxxxx xxxxxxxxx xxx-xxxxxxxxxx xxxxxx xx xxx-xxxxxx"""),
         },

         for xxx_xxxx, xxx_xxxx, xxx_xxxxxxx, xxx_xxxxxxx in xxxxxx_xxxxx:
-            xxxx = (
-                x"""
+            xxxx = x"""
                 ({x.x18x.xxx.xxxxxx_xxxxxx(xxx_xxxx)} xx {x.x18x.xxx.xxxxxx_xxxxxx(xxx_xxxx)})
-                """
-                if xxx_xxxx != xxx_xxxx
-                else x.x18x.xxx.xxxxxx_xxxxxx(xxx_xxxx)
-            )
+                """ if xxx_xxxx != xxx_xxxx else x.x18x.xxx.xxxxxx_xxxxxx(xxx_xxxx)

(Harder to notice the ternary)

cc @aneeshusa as the author of #1879

I'm inclined to remove this feature from the 2024 stable style given this feedback.

These are interesting cases. I did a quick check on how ruff would format these with the new preview style enabled:

VALUE = [
    Thing("""
        value
        """)
]


func(
    (
        value
        + """
        something
        """
    ),
    argument,
)


def foo():
    result = (
        """
        something
        """
        + value
    )


xxxx_xxxxxxx.xxxxxx_xxxxxxxxxxxx_xxxxxx_xx_xxx_xxxxxx: {
    "xxxxx": """Sxxxxx xxxxxxxxxxxx xxx xxxxx (xxxxxx xxx xxxxxxx)""",
    "xxxxxxxx": ("""Sxxxxxxx xxxxxxxx, xxxxxxx xx xxxxxxxxx 
    xxxxxxxxxxxxx xxxxxxx xxxxxxxxx xxx-xxxxxxxxxx xxxxxx xx xxx-xxxxxx"""),
}


for xxx_xxxx, xxx_xxxx, xxx_xxxxxxx, xxx_xxxxxxx in xxxxxx_xxxxx:
    xxxx = (
        """x
    """
        if xxx_xxxx != xxx_xxxx
        else x.x18x.xxx.xxxxxx_xxxxxx(xxx_xxxx)
    )

The reason I checked is because ruff limits where the multiline-string preview style applies:

It only applies it for a single string literal expression (not binary expressions or conditional expression)
It doesn't apply the style recursively (to align with Ruff's hug_parentheses preview style). That's why the array expression for the first example are not on the same line as the call.

I share the sentiment that

Thing("""
    value
    """)

can look a bit ugly. Although that's also the main intend of the change.

I see a few options:

Special case on the functions for which the new style applies. E.g. only apply for dedent but not other functions. I'm not a huge fan of it because it leads to different formatting that's hard to understand for users
Prettier keeps the block indent around multiline strings if the multiline string already started on a new line in the source document and otherwise omits it:
```
 Thing(`
     value
     `);

 Thing(
   `
     value
     `,
 );
```
These examples are the input and expected formatting. Using this heuristic would reduce the diff generated by the new preview style and give users a choice of what looks best for their specific multiline string. The downside is that it's kind of magic and it doesn't enforce consistent formatting.

I share the sentiment that
Thing("""
    value
    """)
can look a bit ugly. Although that's also the main intend of the change.

I understand that's the intention, but my primary complaint is that it breaks the intention of the developer who wrote those multiline strings by unaligning those opening and closing quotes, thereby reducing the user's ability to easily scan and read that block of text.

Having the triple quotes stand alone, aligned with the text inside, makes it far more readable than collapsing it into the surrounding context. I think that's an important distinction to make, and I don't like black making style choices about block quotes that disrupt their readability.

I feel part of the issue is that Black's current and new style demand a different quote alignment, but we can't change the quote alignment inside Black because that would change the contents of the string.

Yeah, I was almost thinking it could be interesting to ship a separate codemod that changes AST to fix this up. Not sure that workflow works well for Black though

@aneeshusa

Appreciate the feedback and recognize the issues here. Wanted to mention the original author of this feature @aneeshusa recently left Lyft, but still wanted to share our perspective. This feature provides a tremendous benefit for Lyft’s codebase and is something we’ve been anticipating for a couple of years now as it fixes the original issue mentioned here: #256.

Looking at the examples, we agree the new formatting isn’t very desirable; at the same time the existing way some multiline strings are formatted is also not ideal. The proposed multiline string changes does what’s possible without modifying the string itself. Given the clash between current/new proposed style, it does seem tricky to come up with a formatting that satisfies both ways.

The main concern is potentially having to wait another year for this feature. Would love to see if there is a way to still move forward with this feature release for 2024.

At this point, it's unfortunately too late to fix the issues with this change; I promised to get the new stable style out by the end of the month. I want people who upgrade to the new stable style see a diff that improves their code, and with this feature I think there's too high a chance that people will instead see their code get worse.

I do agree that in many cases the change makes the code better. We'll have to use this issue to come up with ways to rescue the good parts while minimizing disruptive changes.

Looking at some of these issues:

Thing("""
    value
    """)
VALUE = [
    Thing("""
        value
        """)
]

Manually adding a magic trailing comma to the function call forces Black to expand the function call, aligning it again:

Thing(
    """
    value
    """,
)
VALUE = [
    Thing(
        """
        value
        """,
    )
]

Since one of the main motivators for this change was so that the variable would not be wrapped in textwrap.dedent, there's no other good way to fix this unless Black was to format differently based on the string contents or function name, which doesn't sound like a good idea to me.

I don't believe there's a good way to fix this situation:

func(
    (value + """
        something
        """),
    argument,
)


def foo():
    result = """
        something
        """ + value

There's similar test cases that show operations involving multiline strings at the top level, and they don't include parens:

black/tests/data/cases/preview_multiline_strings.py

Lines 278 to 284 in 5f23701

    
           parser.usage += """ 
        
           Custom extra help summary. 
        
           Extra test: 
        
           - with 
        
           - bullets 
        
           """

black/tests/data/cases/preview_multiline_strings.py

Lines 335 to 343 in 5f23701

    
           dis_c_instance_method = """\ 
        
           %3d           0 LOAD_FAST                1 (x) 
        
                         2 LOAD_CONST               1 (1) 
        
                         4 COMPARE_OP               2 (==) 
        
                         6 LOAD_FAST                0 (self) 
        
                         8 STORE_ATTR               0 (x) 
        
                        10 LOAD_CONST               0 (None) 
        
                        12 RETURN_VALUE 
        
           """ % (_C.__init__.__code__.co_firstlineno + 1,)

So removing the parens here is more consistent.
Here, I think it's best for the user to manually update the indentation to realign it and/or use f-strings -- nothing Black can do to help out here, unless, again, we format differently depending on the context or string contents, which doesn't sound like a good idea. It'd just be some manual work when the user updates Black, then all future strings would end up correctly indented (assuming Black is regularly run).
Alternatively, Black could allow parens here, just not add them by default? Is that a good compromise, or is consistency a higher priority?

To sum up the other issues mentioned, which we can do something about:

Dictionary values with multiline strings should stay parenthesized
Ternaries with multiline strings in the value should stay parenthesized

Merge branch 'main' into psfgh-4159

Navigation Menu

Repository navigation

`multiline_string_handling` sometimes leads to worse formatting #4159

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	parser.usage += """
	Custom extra help summary.

	Extra test:
	- with
	- bullets
	"""

	dis_c_instance_method = """\
	%3d 0 LOAD_FAST 1 (x)
	2 LOAD_CONST 1 (1)
	4 COMPARE_OP 2 (==)
	6 LOAD_FAST 0 (self)
	8 STORE_ATTR 0 (x)
	10 LOAD_CONST 0 (None)
	12 RETURN_VALUE
	""" % (_C.__init__.__code__.co_firstlineno + 1,)

multiline_string_handling sometimes leads to worse formatting #4159

Description

Activity

MichaReiser commented on Jan 22, 2024

amyreese commented on Jan 22, 2024

JelleZijlstra commented on Jan 22, 2024

hauntsaninja commented on Jan 22, 2024

yuhrichard commented on Jan 23, 2024

JelleZijlstra commented on Jan 24, 2024

cobaltt7 commented on Feb 17, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

`multiline_string_handling` sometimes leads to worse formatting #4159