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

Add parens around implicit string concatenations where increases readability #3162

Merged
merged 7 commits into from
Aug 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
normalized as expected (#3168)
- When using `--skip-magic-trailing-comma` or `-C`, trailing commas are stripped from
subscript expressions with more than 1 element (#3209)
- Implicitly concatenated strings inside a list, set, or tuple are now wrapped inside
parentheses (#3162)
- Fix a string merging/split issue when a comment is present in the middle of implicitly
concatenated strings on its own line (#3227)

Expand Down
50 changes: 48 additions & 2 deletions src/black/trans.py
Original file line number Diff line number Diff line change
Expand Up @@ -1043,6 +1043,41 @@ def _get_max_string_length(self, line: Line, string_idx: int) -> int:
max_string_length = self.line_length - offset
return max_string_length

@staticmethod
def _prefer_paren_wrap_match(LL: List[Leaf]) -> Optional[int]:
"""
Returns:
string_idx such that @LL[string_idx] is equal to our target (i.e.
matched) string, if this line matches the "prefer paren wrap" statement
requirements listed in the 'Requirements' section of the StringParenWrapper
class's docstring.
OR
None, otherwise.
"""
# The line must start with a string.
if LL[0].type != token.STRING:
return None

matching_nodes = [
syms.listmaker,
syms.dictsetmaker,
syms.testlist_gexp,
]
# If the string is an immediate child of a list/set/tuple literal...
if (
parent_type(LL[0]) in matching_nodes
or parent_type(LL[0].parent) in matching_nodes
):
# And the string is surrounded by commas (or is the first/last child)...
prev_sibling = LL[0].prev_sibling
next_sibling = LL[0].next_sibling
if (not prev_sibling or prev_sibling.type == token.COMMA) and (
not next_sibling or next_sibling.type == token.COMMA
):
return 0

return None


def iter_fexpr_spans(s: str) -> Iterator[Tuple[int, int]]:
"""
Expand Down Expand Up @@ -1138,6 +1173,9 @@ class StringSplitter(BaseStringSplitter, CustomSplitMapMixin):
def do_splitter_match(self, line: Line) -> TMatchResult:
LL = line.leaves

if self._prefer_paren_wrap_match(LL) is not None:
return TErr("Line needs to be wrapped in parens first.")

is_valid_index = is_valid_index_factory(LL)

idx = 0
Expand Down Expand Up @@ -1583,8 +1621,7 @@ def _get_string_operator_leaves(self, leaves: Iterable[Leaf]) -> List[Leaf]:

class StringParenWrapper(BaseStringSplitter, CustomSplitMapMixin):
"""
StringTransformer that splits non-"atom" strings (i.e. strings that do not
exist on lines by themselves).
StringTransformer that wraps strings in parens and then splits at the LPAR.

Requirements:
All of the requirements listed in BaseStringSplitter's docstring in
Expand All @@ -1604,6 +1641,11 @@ class StringParenWrapper(BaseStringSplitter, CustomSplitMapMixin):
OR
* The line is a dictionary key assignment where some valid key is being
assigned the value of some string.
OR
* The line starts with an "atom" string that prefers to be wrapped in
parens. It's preferred to be wrapped when it's is an immediate child of
a list/set/tuple literal, AND the string is surrounded by commas (or is
the first/last child).

Transformations:
The chosen string is wrapped in parentheses and then split at the LPAR.
Expand All @@ -1628,6 +1670,9 @@ class StringParenWrapper(BaseStringSplitter, CustomSplitMapMixin):
changed such that it no longer needs to be given its own line,
StringParenWrapper relies on StringParenStripper to clean up the
parentheses it created.

For "atom" strings that prefers to be wrapped in parens, it requires
StringSplitter to hold the split until the string is wrapped in parens.
"""

def do_splitter_match(self, line: Line) -> TMatchResult:
Expand All @@ -1644,6 +1689,7 @@ def do_splitter_match(self, line: Line) -> TMatchResult:
or self._assert_match(LL)
or self._assign_match(LL)
or self._dict_match(LL)
or self._prefer_paren_wrap_match(LL)
)

if string_idx is not None:
Expand Down
46 changes: 30 additions & 16 deletions tests/data/preview/comments7.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,39 +226,53 @@ class C:
# metadata_version errors.
(
{},
"None is an invalid value for Metadata-Version. Error: This field is"
" required. see"
" https://packaging.python.org/specifications/core-metadata",
(
"None is an invalid value for Metadata-Version. Error: This field"
" is required. see"
" https://packaging.python.org/specifications/core-metadata"
),
),
(
{"metadata_version": "-1"},
"'-1' is an invalid value for Metadata-Version. Error: Unknown Metadata"
" Version see"
" https://packaging.python.org/specifications/core-metadata",
(
"'-1' is an invalid value for Metadata-Version. Error: Unknown"
" Metadata Version see"
" https://packaging.python.org/specifications/core-metadata"
),
),
# name errors.
(
{"metadata_version": "1.2"},
"'' is an invalid value for Name. Error: This field is required. see"
" https://packaging.python.org/specifications/core-metadata",
(
"'' is an invalid value for Name. Error: This field is required."
" see https://packaging.python.org/specifications/core-metadata"
),
),
(
{"metadata_version": "1.2", "name": "foo-"},
"'foo-' is an invalid value for Name. Error: Must start and end with a"
" letter or numeral and contain only ascii numeric and '.', '_' and"
" '-'. see https://packaging.python.org/specifications/core-metadata",
(
"'foo-' is an invalid value for Name. Error: Must start and end"
" with a letter or numeral and contain only ascii numeric and '.',"
" '_' and '-'. see"
" https://packaging.python.org/specifications/core-metadata"
),
),
# version errors.
(
{"metadata_version": "1.2", "name": "example"},
"'' is an invalid value for Version. Error: This field is required. see"
" https://packaging.python.org/specifications/core-metadata",
(
"'' is an invalid value for Version. Error: This field is required."
" see https://packaging.python.org/specifications/core-metadata"
),
),
(
{"metadata_version": "1.2", "name": "example", "version": "dog"},
"'dog' is an invalid value for Version. Error: Must start and end with"
" a letter or numeral and contain only ascii numeric and '.', '_' and"
" '-'. see https://packaging.python.org/specifications/core-metadata",
(
"'dog' is an invalid value for Version. Error: Must start and end"
" with a letter or numeral and contain only ascii numeric and '.',"
" '_' and '-'. see"
" https://packaging.python.org/specifications/core-metadata"
),
),
],
)
Expand Down
81 changes: 79 additions & 2 deletions tests/data/preview/long_strings.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@

D4 = {"A long and ridiculous {}".format(string_key): "This is a really really really long string that has to go i,side of a dictionary. It is soooo bad.", some_func("calling", "some", "stuff"): "This is a really really really long string that has to go inside of a dictionary. It is {soooo} bad (#{x}).".format(sooo="soooo", x=2), "A %s %s" % ("formatted", "string"): "This is a really really really long string that has to go inside of a dictionary. It is %s bad (#%d)." % ("soooo", 2)}

L1 = ["The is a short string", "This is a really long string that can't possibly be expected to fit all together on one line. Also it is inside a list literal, so it's expected to be wrapped in parens when spliting to avoid implicit str concatenation.", short_call("arg", {"key": "value"}), "This is another really really (not really) long string that also can't be expected to fit on one line and is, like the other string, inside a list literal.", ("parens should be stripped for short string in list")]

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

S1 = {"The is a short string", "This is a really long string that can't possibly be expected to fit all together on one line. Also it is inside a set literal, so it's expected to be wrapped in parens when spliting to avoid implicit str concatenation.", short_call("arg", {"key": "value"}), "This is another really really (not really) long string that also can't be expected to fit on one line and is, like the other string, inside a set literal.", ("parens should be stripped for short string in set")}

S2 = {"This is a really long string that can't be expected to fit in one line and is the only child of a set literal."}

T1 = ("The is a short string", "This is a really long string that can't possibly be expected to fit all together on one line. Also it is inside a tuple literal, so it's expected to be wrapped in parens when spliting to avoid implicit str concatenation.", short_call("arg", {"key": "value"}), "This is another really really (not really) long string that also can't be expected to fit on one line and is, like the other string, inside a tuple literal.", ("parens should be stripped for short string in list"))

T2 = ("This is a really long string that can't be expected to fit in one line and is the only child of a tuple literal.",)

func_with_keywords(my_arg, my_kwarg="Long keyword strings also need to be wrapped, but they will probably need to be handled a little bit differently.")

bad_split1 = (
Expand Down Expand Up @@ -109,7 +121,7 @@

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.

arg_comment_string = print("Long lines with inline comments which are apart of (and not the only member of) an argument list should have their comments appended to the reformatted string's enclosing left parentheses.", # This comment stays on the bottom.
arg_comment_string = print("Long lines with inline comments which are apart of (and not the only member of) an argument list should have their comments appended to the reformatted string's enclosing left parentheses.", # This comment gets thrown to the top.
"Arg #2", "Arg #3", "Arg #4", "Arg #5")

pragma_comment_string1 = "Lines which end with an inline pragma comment of the form `# <pragma>: <...>` should be left alone." # noqa: E501
Expand Down Expand Up @@ -345,6 +357,71 @@ def foo():
% ("soooo", 2),
}

L1 = [
"The is a short string",
(
"This is a really long string that can't possibly be expected to fit all"
" together on one line. Also it is inside a list literal, so it's expected to"
" be wrapped in parens when spliting to avoid implicit str concatenation."
),
short_call("arg", {"key": "value"}),
(
"This is another really really (not really) long string that also can't be"
" expected to fit on one line and is, like the other string, inside a list"
" literal."
),
"parens should be stripped for short string in list",
]

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."
]
ichard26 marked this conversation as resolved.
Show resolved Hide resolved

S1 = {
"The is a short string",
(
"This is a really long string that can't possibly be expected to fit all"
" together on one line. Also it is inside a set literal, so it's expected to be"
" wrapped in parens when spliting to avoid implicit str concatenation."
),
short_call("arg", {"key": "value"}),
(
"This is another really really (not really) long string that also can't be"
" expected to fit on one line and is, like the other string, inside a set"
" literal."
),
"parens should be stripped for short string in set",
}

S2 = {
"This is a really long string that can't be expected to fit in one line and is the"
" only child of a set literal."
}

T1 = (
"The is a short string",
(
"This is a really long string that can't possibly be expected to fit all"
" together on one line. Also it is inside a tuple literal, so it's expected to"
" be wrapped in parens when spliting to avoid implicit str concatenation."
),
short_call("arg", {"key": "value"}),
(
"This is another really really (not really) long string that also can't be"
" expected to fit on one line and is, like the other string, inside a tuple"
" literal."
),
"parens should be stripped for short string in list",
)

T2 = (
(
"This is a really long string that can't be expected to fit in one line and is"
" the only child of a tuple literal."
),
)

func_with_keywords(
my_arg,
my_kwarg=(
Expand Down Expand Up @@ -487,7 +564,7 @@ def foo():
arg_comment_string = print(
"Long lines with inline comments which are apart of (and not the only member of) an"
" argument list should have their comments appended to the reformatted string's"
" enclosing left parentheses.", # This comment stays on the bottom.
" enclosing left parentheses.", # This comment gets thrown to the top.
"Arg #2",
"Arg #3",
"Arg #4",
Expand Down
32 changes: 20 additions & 12 deletions tests/data/preview/long_strings__regression.py
Original file line number Diff line number Diff line change
Expand Up @@ -763,20 +763,28 @@ def xxxx_xxx_xx_xxxxxxxxxx_xxxx_xxxxxxxxx(xxxx):

some_dictionary = {
"xxxxx006": [
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx0xx6xxxxxxxxxx2xxxxxx9xxxxxxxxxx0xxxxx1xxx2x/xx9xx6+x+xxxxxxxxxxxxxx4xxxxxxxxxxxxxxxxxxxxx43xxx2xx2x4x++xxx6xxxxxxxxx+xxxxx/xx9x+xxxxxxxxxxxxxx8x15xxxxxxxxxxxxxxxxx82xx/xxxxxxxxxxxxxx/x5xxxxxxxxxxxxxx6xxxxxx74x4/xxx4x+xxxxxxxxx2xxxxxxxx87xxxxx4xxxxxxxx3xx0xxxxx4xxx1xx9xx5xxxxxxx/xxxxx5xx6xx4xxxx1x/x2xxxxxxxxxxxx64xxxxxxx1x0xx5xxxxxxxxxxxxxx=="
" xxxxx000 xxxxxxxxxx\n",
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx6xxxxxxxxxxxxxx9xxxxxxxxxxxxx3xxx9xxxxxxxxxxxxxxxx0xxxxxxxxxxxxxxxxx2xxxx2xxx6xxxxx/xx54xxxxxxxxx4xxx3xxxxxx9xx3xxxxx39xxxxxxxxx5xx91xxxx7xxxxxx8xxxxxxxxxxxxxxxx9xxx93xxxxxxxxxxxxxxxxx7xxx8xx8xx4/x1xxxxx1x3xxxxxxxxxxxxx3xxxxxx9xx4xx4x7xxxxxxxxxxxxx1xxxxxxxxx7xxxxxxxxxxxxxx4xx6xxxxxxxxx9xxx7xxxx2xxxxxxxxxxxxxxxxxxxxxx8xxxxxxxxxxxxxxxxxxxx6xx=="
" xxxxx010 xxxxxxxxxx\n",
(
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx0xx6xxxxxxxxxx2xxxxxx9xxxxxxxxxx0xxxxx1xxx2x/xx9xx6+x+xxxxxxxxxxxxxx4xxxxxxxxxxxxxxxxxxxxx43xxx2xx2x4x++xxx6xxxxxxxxx+xxxxx/xx9x+xxxxxxxxxxxxxx8x15xxxxxxxxxxxxxxxxx82xx/xxxxxxxxxxxxxx/x5xxxxxxxxxxxxxx6xxxxxx74x4/xxx4x+xxxxxxxxx2xxxxxxxx87xxxxx4xxxxxxxx3xx0xxxxx4xxx1xx9xx5xxxxxxx/xxxxx5xx6xx4xxxx1x/x2xxxxxxxxxxxx64xxxxxxx1x0xx5xxxxxxxxxxxxxx=="
" xxxxx000 xxxxxxxxxx\n"
),
(
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx6xxxxxxxxxxxxxx9xxxxxxxxxxxxx3xxx9xxxxxxxxxxxxxxxx0xxxxxxxxxxxxxxxxx2xxxx2xxx6xxxxx/xx54xxxxxxxxx4xxx3xxxxxx9xx3xxxxx39xxxxxxxxx5xx91xxxx7xxxxxx8xxxxxxxxxxxxxxxx9xxx93xxxxxxxxxxxxxxxxx7xxx8xx8xx4/x1xxxxx1x3xxxxxxxxxxxxx3xxxxxx9xx4xx4x7xxxxxxxxxxxxx1xxxxxxxxx7xxxxxxxxxxxxxx4xx6xxxxxxxxx9xxx7xxxx2xxxxxxxxxxxxxxxxxxxxxx8xxxxxxxxxxxxxxxxxxxx6xx=="
" xxxxx010 xxxxxxxxxx\n"
),
],
"xxxxx016": [
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx0xx6xxxxxxxxxx2xxxxxx9xxxxxxxxxx0xxxxx1xxx2x/xx9xx6+x+xxxxxxxxxxxxxx4xxxxxxxxxxxxxxxxxxxxx43xxx2xx2x4x++xxx6xxxxxxxxx+xxxxx/xx9x+xxxxxxxxxxxxxx8x15xxxxxxxxxxxxxxxxx82xx/xxxxxxxxxxxxxx/x5xxxxxxxxxxxxxx6xxxxxx74x4/xxx4x+xxxxxxxxx2xxxxxxxx87xxxxx4xxxxxxxx3xx0xxxxx4xxx1xx9xx5xxxxxxx/xxxxx5xx6xx4xxxx1x/x2xxxxxxxxxxxx64xxxxxxx1x0xx5xxxxxxxxxxxxxx=="
" xxxxx000 xxxxxxxxxx\n",
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx6xxxxxxxxxxxxxx9xxxxxxxxxxxxx3xxx9xxxxxxxxxxxxxxxx0xxxxxxxxxxxxxxxxx2xxxx2xxx6xxxxx/xx54xxxxxxxxx4xxx3xxxxxx9xx3xxxxx39xxxxxxxxx5xx91xxxx7xxxxxx8xxxxxxxxxxxxxxxx9xxx93xxxxxxxxxxxxxxxxx7xxx8xx8xx4/x1xxxxx1x3xxxxxxxxxxxxx3xxxxxx9xx4xx4x7xxxxxxxxxxxxx1xxxxxxxxx7xxxxxxxxxxxxxx4xx6xxxxxxxxx9xxx7xxxx2xxxxxxxxxxxxxxxxxxxxxx8xxxxxxxxxxxxxxxxxxxx6xx=="
" xxxxx010 xxxxxxxxxx\n",
(
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx0xx6xxxxxxxxxx2xxxxxx9xxxxxxxxxx0xxxxx1xxx2x/xx9xx6+x+xxxxxxxxxxxxxx4xxxxxxxxxxxxxxxxxxxxx43xxx2xx2x4x++xxx6xxxxxxxxx+xxxxx/xx9x+xxxxxxxxxxxxxx8x15xxxxxxxxxxxxxxxxx82xx/xxxxxxxxxxxxxx/x5xxxxxxxxxxxxxx6xxxxxx74x4/xxx4x+xxxxxxxxx2xxxxxxxx87xxxxx4xxxxxxxx3xx0xxxxx4xxx1xx9xx5xxxxxxx/xxxxx5xx6xx4xxxx1x/x2xxxxxxxxxxxx64xxxxxxx1x0xx5xxxxxxxxxxxxxx=="
" xxxxx000 xxxxxxxxxx\n"
),
(
"xxx-xxx"
" xxxxx3xxxx1xx2xxxxxxxxxxxxxx6xxxxxxxxxxxxxx9xxxxxxxxxxxxx3xxx9xxxxxxxxxxxxxxxx0xxxxxxxxxxxxxxxxx2xxxx2xxx6xxxxx/xx54xxxxxxxxx4xxx3xxxxxx9xx3xxxxx39xxxxxxxxx5xx91xxxx7xxxxxx8xxxxxxxxxxxxxxxx9xxx93xxxxxxxxxxxxxxxxx7xxx8xx8xx4/x1xxxxx1x3xxxxxxxxxxxxx3xxxxxx9xx4xx4x7xxxxxxxxxxxxx1xxxxxxxxx7xxxxxxxxxxxxxx4xx6xxxxxxxxx9xxx7xxxx2xxxxxxxxxxxxxxxxxxxxxx8xxxxxxxxxxxxxxxxxxxx6xx=="
" xxxxx010 xxxxxxxxxx\n"
),
],
}

Expand Down
Loading