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

Black is not removing unnecessary + signs in concatenated strings #2268

Open
tmotyl opened this issue May 28, 2021 · 6 comments
Open

Black is not removing unnecessary + signs in concatenated strings #2268

tmotyl opened this issue May 28, 2021 · 6 comments
Labels
F: strings Related to our handling of strings T: style What do we want Blackened code to look like?

Comments

@tmotyl
Copy link

tmotyl commented May 28, 2021

Describe the bug A clear and concise description of what the bug is.

second_string = ("def"
    + "ddd" + "ddd")

is formated to

second_string = "def" + "ddd" + "ddd"

where I would expect:

second_string = "defdddddd"

To Reproduce Steps to reproduce the behavior:

https://black.vercel.app/?version=main&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4ADPAHpdAD2IimZxl1N_Wl9XM4Hukmv0lHvH8ZV8H_6vvOURMPls_WzMNcku5-VbAQaYsBSET4vbhPFAzFY5DcRsFuGPJMGD-DLKFHBbxUtuKRJa5J0G0_Yx5DOsXpPSCHuM1nMZEs7HyKRNJp5Z55EyifvKOiVnNdKMFmwsCkKQAAAAd1B6wN16dJoAAZYB0AEAAJ3bnBKxxGf7AgAAAAAEWVo=

Expected behavior A clear and concise description of what you expected to happen.

Environment (please complete the following information):

  • Version: main

Does this bug also happen on main? To answer this, you have two options:
yes

@felix-hilden
Copy link
Collaborator

Moreover, as presented in your example, strings on the same line originally are not concatenated either.

long_string = "abc" + "def"

Now this functionality is not to be expected, but... given that the feature is advertised (and I was the one that wrote the damn paragraph in #2106, sigh 😄), and I can't make it happen with the --experimental-string-processing flag, this should be addressed in one of two ways:

  • Implement the functionality (I'd say a reasonable suggestion)
  • Clarify the documentation to say that only parenthesised strings are concatenated

Playground, but this works locally as expected with --experimental-string-processing. What do you guys think?

@ichard26 ichard26 added F: strings Related to our handling of strings T: style What do we want Blackened code to look like? labels May 28, 2021
@avalanchy
Copy link

Maybe the idea is cool, but it's not something that should be part of Black. AFAIK Black does do not touch the behavior and here we remove the concatenation operation.

@JelleZijlstra
Copy link
Collaborator

It's fine to do for string literals. The compiler does this already anyway.

In [196]: dis.dis('"a" + "b"')
  1           0 LOAD_CONST               2 ('ab')
              2 RETURN_VALUE

@ItsCinnabar
Copy link

Maybe the idea is cool, but it's not something that should be part of Black. AFAIK Black does do not touch the behavior and here we remove the concatenation operation.

Agreed. I personally don't think this is something black should do.

@felix-hilden
Copy link
Collaborator

Just to note, this is the polar opposite of #2553.

@yilei
Copy link
Contributor

yilei commented Sep 27, 2022

Hmm, this isn't necessarily the opposite of #2553.

There are a few options that they can co-exist:

  1. We could restrict this to cases where they are within the line length.
  2. Or, when processing strings, we merge explicitly or implicitly concatenated strings first, then when performing the split (because it exceeds line length), we use explicit str concatenation.

Either case, the initial example could still be formatted to second_string = "defdddddd".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: strings Related to our handling of strings T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

7 participants