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 tuple.

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 ...

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

[github-actions]

diff-shades results comparing this PR (3c8cd3a) to main (21218b6). 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]

hm, it seems like this comment gets moved to the beginning? That's unfortunate.

[yilei]

yeah, i'm actually not sure where the comment should go. there is another existing example:

black/tests/data/preview/long_strings.py

Line 91 in 1b6de7b

comment_string = "Long lines with inline comments should have their comments appended to the reformatted string's enclosing right parentheses." # This comment gets thrown to the top.

black/tests/data/preview/long_strings.py

Lines 447 to 450 in 1b6de7b

	comment_string = ( # This comment gets thrown to the top.
	"Long lines with inline comments should have their comments appended to the"
	" reformatted string's enclosing right parentheses."
	)

that throws the comment to the opening paren, which is the same behavior here.

looks like it was introduced in #1132, but it's unclear whether this behavior is intentionally designed or not.

thoughts?

[JelleZijlstra]

I see, thanks. Maybe we can fix this, but seems like the issue is independent from this PR.

[ichard26]

I agree that this improves formatting. The code looks mostly good, but noticed few issues:

• Tuple literals are not considered, that seems unconsistent
• Wrapping broken up string literals that live alone in a sequence literal that simply has a trailing comma seems overkill?

--- test.py	2022-08-03 17:43:27.228805 +0000
+++ test.py	2022-08-03 17:43:28.373631 +0000
@@ -1,8 +1,10 @@
 L2 = [
-    "This is a really long string that can't be expected to fit in one line and is the "
-    "only child of a list literal.",
+    (
+        "This is a really long string that can't be expected to fit in one line and is"
+        " the only child of a list literal."
+    ),
 ]

--- a/django:django/db/backends/mysql/features.py
+++ b/django:django/db/backends/mysql/features.py
@@ -122,12 +132,14 @@
             skips.update(
                 {
                     "https://jira.mariadb.org/browse/MDEV-19598": {
-                        "schema.tests.SchemaTests."
-                        "test_alter_not_unique_field_to_primary_key",
+                        (
+                            "schema.tests.SchemaTests."
+                            "test_alter_not_unique_field_to_primary_key"
+                        ),
                     },
                 }
             )

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

CompiledSQL(
    "SELECT addresses.user_id AS addresses_user_id, "
    "addresses.email_address AS addresses_email_address "
    # note the comma-separated FROM clause
    "FROM addresses, (SELECT addresses_1.id AS id FROM "
    "AND addresses.id = anon_1.id ORDER BY addresses.id",
    arg_func(),
)

--- test.py	2022-08-03 18:54:22.054128 +0000
+++ test.py	2022-08-03 18:57:45.657305 +0000
@@ -1,8 +1,9 @@
 CompiledSQL(
-    "SELECT addresses.user_id AS addresses_user_id, "
-    "addresses.email_address AS addresses_email_address "
+    (
+        "SELECT addresses.user_id AS addresses_user_id, "
+        "addresses.email_address AS addresses_email_address "
+    )
     # note the comma-separated FROM clause
-    "FROM addresses, (SELECT addresses_1.id AS id FROM "
-    "AND addresses.id = anon_1.id ORDER BY addresses.id",
+    "FROM addresses, (SELECT addresses_1.id AS id FROM AND addresses.id = anon_1.id ORDER BY addresses.id",
     arg_func(),
 )

(note that due to the forced second pass in format_str, the error output from Black is less than helpful, it claims the input is invalid, but really the input to the second pass was invalid because the first pass made it invalid ... also the odd merge of the last two string literals is #2734, it's unrelated to your PR)

[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! 🏖️

[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 #2734. so, i'll send a separate PR to fix #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 #3162 (comment)

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 #3227

[ichard26]

The amount of changes diff-shades reports for this PR scares me, especially since I'm not entirely sure if it's really an improvement in situations like these:

--- a/warehouse:warehouse/packaging/services.py
+++ b/warehouse:warehouse/packaging/services.py
@@ -33,13 +33,15 @@
         # This class should not be used in production, it's trivial for it to
         # be used to read arbitrary files from the disk. It is intended ONLY
         # for local development with trusted users. To make this clear, we'll
         # raise a warning.
         warnings.warn(
-            "LocalFileStorage is intended only for use in development, you "
-            "should not use it in production due to the lack of safe guards "
-            "for safely locating files on disk.",
+            (
+                "LocalFileStorage is intended only for use in development, you "
+                "should not use it in production due to the lack of safe guards "
+                "for safely locating files on disk."
+            ),
             InsecureStorageWarning,
         )

To be fair, the extra parenthesises and indention are mostly fine, but seem redundant. Probably doesn't make much sense to add even more complexity to the "certain contexts" part of this formatting rule though to keep stuff like this unchanged. Checking whether the string's previous (previous, cause the comma) or next (next) sibling is also a string sounds incredibly fragile.

[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"
    ),
}

[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

[ichard26]

Looks way better!

Still iffy about stuff like this:

django_test_skips = {
    "opclasses are PostgreSQL only.": {
        (
            "indexes.tests.SchemaIndexesNotPostgreSQLTests."
            "test_create_index_ignores_opclasses"
        ),
    },
}

but as long as you can remove the trailing comma and Black collapses the wrapping parentheses, it's not a big problem.

Thank you so much for working and sticking with this through the multiple rounds of review! Cheers to another merged PR! 🎉

[yilei]

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