Add parens around implicit string concatenations where increases readability

[yilei]

Description

This PR resolves #3159. It adds parentheses around implicit string concatenations when it's inside a list, set, or function call.

Implementation notes

Looking at the order of the transformers here, we need to "wrap in parens" before string_split runs. So my solution is to introduce a "collaboration" between StringSplitter and StringParenWrapper where the splitter "skips" the split until the wrapper adds the parens (and then the line after the paren is split by StringSplitter) in another pass.

I have also considered an alternative approach, where I tried to add a different "string paren wrapper" class, and it runs before string_split. Then I found out it requires a different do_transform implementation than StringParenWrapper.do_transform, since the later assumes it runs after the delimiter_split transform. So I stopped researching that route.

Checklist - did you ...

• [x] Add a CHANGELOG entry if necessary?
• [x] Add / update tests if necessary?
• [x] Add new / update outdated documentation?

[github-actions[bot]]

diff-shades results comparing this PR (3c8cd3abdfce95fd919d9560ac844ea6d7b0dffa) to main (21218b666aeafd1c089cbe998e730f97605d25b2). The full diff is available in the logs under the "Generate HTML diff report" step.

╭────────────────────────── Summary ───────────────────────────╮
│ 12 projects & 141 files changed / 3765 changes [+2377/-1388] │
│                                                              │
│ ... out of 2 260 861 lines, 10 396 files & 23 projects       │
╰──────────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

[yilei]

I scanned through the diff-shades results, they all look expected to this PR's change.

[Jackenmen]

I scanned through the diff-shades results, they all look expected to this PR's change.

You have to put it behind the --preview flag as assert-no-changes rightfully fails as your PR is currently affecting the stable style.

[yilei]

@jack1142 The changed code is already behind the --preview flag (if Preview.string_processing in mode:), the diff from assert-no-changes is from https://github.com/ichard26/blackbench, which enables it via the (deprecated) --experimental-string-processing flag.

I assume this check doesn't force it to be false, or am I missing something?

[ichard26]

Gah, I missed that >.<

Instead of fixing diff-shades, I'll just push a quick update to blackbench to switch to the preview option 'cause I maintain all of the black devtooling :P

[yilei]

Thank you @ichard26!

Adding the new --preview diff report link for convenience: https://github.com/psf/black/runs/7313506857?check_suite_focus=true#step:10:1

[JelleZijlstra]

• Wrapping broken up string literals that live alone in a sequence literal that simply has a trailing comma seems overkill?

I think the parentheses are actually helpful here, because otherwise the code looks like a two-element list.

[yilei]

quick status update: i'm currently on vacation. will address the comments in about two weeks. thanks for pointing out the issues!

[ichard26]

enjoy your vacation! :beach_umbrella:

[yilei]

looking at a fix for the last "comment in the middle of strings" issue, to actually wrap the entire string + comment in parens is really a hard thing to do given the complexity of STANDALONE_COMMENT.

however, i think one reasonable option is to skip wrapping in parens for this case. to do that, the code change required seems to also fix the mentioned https://github.com/psf/black/issues/2734. so, i'll send a separate PR to fix https://github.com/psf/black/issues/2734 first.

[yilei]

@ichard26 I've addressed your comments, please take another look:

Tuple literals are not considered, that seems unconsistent

Tuples are now considered too

Wrapping broken up string literals that live alone in a sequence literal that simply has a trailing comma seems overkill?

Working as intended per https://github.com/psf/black/pull/3162#issuecomment-1204392015

It's wrapping broken up strings literals incorrectly when there's a comment in the middle, breaking valid code

This PR doesn't wrap those cases in parens, and the original incorrect results were fixed by https://github.com/psf/black/pull/3227

[yilei]

Agreed that this is causing scary amount of diffs.

Going back to the original intention of this PR, it tries to avoid bugs for implicit string concatenations that only commonly happen in lists/tuples/sets. As it's quite easy to miss commas:

some_list = [
    " lorem ipsum dolor sit amet consectetur adipiscing elit sed do eiusmod tempor"
    " incididunt ut labore et dolore magna aliqua Ut enim ad minim",
    " veniam quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo",
]

In function calls, this is less of an issue because missing commas should usually result in an immediate runtime error because of argument mis-match.

So, another option is to limit this PR to only list/tuple/set literals and exclude function calls? What do you think, @ichard26 @JelleZijlstra ?

[JelleZijlstra]

So, another option is to limit this PR to only list/tuple/set literals and exclude function calls? What do you think, @ichard26 @JelleZijlstra ?

Yes, I think that's a good idea. Maybe also dicts? Something like

{
    "x": "some"
    "long string",
}

is more readable as

{
    "x": (
        "some"
        "long string"
    ),
}

[JelleZijlstra]

So, another option is to limit this PR to only list/tuple/set literals and exclude function calls? What do you think, @ichard26 @JelleZijlstra ?

Yes, I think that's a good idea. Maybe also dicts? Something like

{
    "x": "some"
    "long string",
}

is more readable as

{
    "x": (
        "some"
        "long string"
    ),
}

[yilei]

Yes, I think that's a good idea

Great, I pushed new commits now this PR no longer wraps function call args.

Maybe also dicts?

You meant the wrapped example is more readable right? This is the value in dicts, which is already done by the current version of black --preview: https://black.vercel.app/?version=stable&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4ADvAHJdAD2IimZxl1N_Wm_vzTQ-P7e9kejh_LvEiUE6tLUAZ1xVz5lZ6m-wo9D4ahShh_wm_7SxyQe1Rl-WZPSnzPI_DUNfuxeWB-Y41DUdrRGpmMMlfFExIzlSN1HqyMjBQ4vDQ-0a1ylSwHDuAyj3rp34nKMCAAAAAB6xJMz7xPYdAAGOAfABAABV9CtdscRn-wIAAAAABFla

[yilei]

Thank you for the thorough review and accepting my PR, I really appreciate it!