-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
multiline_string_handling
sometimes leads to worse formatting
#4159
Comments
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:
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:
|
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 |
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. |
Reports from @amyreese in #4042 (comment):
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:
(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.
The text was updated successfully, but these errors were encountered: