Don't use optional parentheses in unnecessary situations

If an expression starts or ends with a bracket and only contains a single
delimiter, don't wrap it in additional optional parentheses.  We can use the
brackets for the split.

Fixes pytest-dev#177

Fixes pytest-dev#193

README.md

@@ -597,6 +597,9 @@ More details can be found in [CONTRIBUTING](CONTRIBUTING.md).
 * math operators now use their respective priorities for delimiting multiline
   expressions (#148)
 
+* optional parentheses are now omitted on expressions that start or end
+  with a bracket and only contain a single operator (#177)
+
 * empty parentheses in a class definition are now removed (#145, #180)
 
 * string prefixes are now standardized to lowercase and `u` is removed
@@ -621,6 +624,8 @@ More details can be found in [CONTRIBUTING](CONTRIBUTING.md).
   where used both in function signatures with stars and function calls
   with stars but the former would be reformatted to a single line.
 
+* fixed crash on dealing with optional parentheses (#193)
+
 * fixed crash when dead symlinks where encountered
 
 

black.py

@@ -239,11 +239,8 @@ def reformat_one(
                 src = src.resolve()
                 if src in cache and cache[src] == get_cache_info(src):
                     changed = Changed.CACHED
-            if (
-                changed is not Changed.CACHED
-                and format_file_in_place(
-                    src, line_length=line_length, fast=fast, write_back=write_back
-                )
+            if changed is not Changed.CACHED and format_file_in_place(
+                src, line_length=line_length, fast=fast, write_back=write_back
             ):
                 changed = Changed.YES
             if write_back == WriteBack.YES and changed is not Changed.NO:
@@ -860,14 +857,11 @@ def is_def(self) -> bool:
             second_leaf: Optional[Leaf] = self.leaves[1]
         except IndexError:
             second_leaf = None
-        return (
-            (first_leaf.type == token.NAME and first_leaf.value == "def")
-            or (
-                first_leaf.type == token.ASYNC
-                and second_leaf is not None
-                and second_leaf.type == token.NAME
-                and second_leaf.value == "def"
-            )
+        return (first_leaf.type == token.NAME and first_leaf.value == "def") or (
+            first_leaf.type == token.ASYNC
+            and second_leaf is not None
+            and second_leaf.type == token.NAME
+            and second_leaf.value == "def"
         )
 
     @property
@@ -1032,9 +1026,8 @@ def is_complex_subscript(self, leaf: Leaf) -> bool:
             and subscript_start.type == syms.subscriptlist
         ):
             subscript_start = child_towards(subscript_start, leaf)
-        return (
-            subscript_start is not None
-            and any(n.type in TEST_DESCENDANTS for n in subscript_start.pre_order())
+        return subscript_start is not None and any(
+            n.type in TEST_DESCENDANTS for n in subscript_start.pre_order()
         )
 
     def __str__(self) -> str:
@@ -1999,8 +1992,9 @@ def right_hand_split(
     # Since body is a new indent level, remove spurious leading whitespace.
     if body_leaves:
         normalize_prefix(body_leaves[0], inside_brackets=True)
-    elif not head_leaves:
-        # No `head` and no `body` means the split failed. `tail` has all content.
+    if not head_leaves:
+        # No `head` means the split failed. Either `tail` has all content or
+        # the matching `opening_bracket` wasn't available on `line` anymore.
         raise CannotSplit("No brackets found")
 
     # Build the new lines.
@@ -2018,19 +2012,27 @@ def right_hand_split(
         # the closing bracket is an optional paren
         and closing_bracket.type == token.RPAR
         and not closing_bracket.value
-        # there are no delimiters or standalone comments in the body
-        and not body.bracket_tracker.delimiters
+        # there are no standalone comments in the body
         and not line.contains_standalone_comments(0)
         # and it's not an import (optional parens are the only thing we can split
         # on in this case; attempting a split without them is a waste of time)
         and not line.is_import
     ):
         omit = {id(closing_bracket), *omit}
-        try:
-            yield from right_hand_split(line, py36=py36, omit=omit)
-            return
-        except CannotSplit:
-            pass
+        delimiter_count = len(body.bracket_tracker.delimiters)
+        if (
+            delimiter_count == 0
+            or delimiter_count == 1
+            and (
+                body.leaves[0].type in OPENING_BRACKETS
+                or body.leaves[-1].type in CLOSING_BRACKETS
+            )
+        ):
+            try:
+                yield from right_hand_split(line, py36=py36, omit=omit)
+                return
+            except CannotSplit:
+                pass
 
     ensure_visible(opening_bracket)
     ensure_visible(closing_bracket)

tests/composition.py

@@ -33,7 +33,7 @@ def test(self) -> None:
                     ):
                         print(i)
 
-    def omitting_trailers() -> None:
+    def omitting_trailers(self) -> None:
         get_collection(
             hey_this_is_a_very_long_call, it_has_funny_attributes, really=True
         )[OneLevelIndex]
@@ -43,3 +43,111 @@ def omitting_trailers() -> None:
         d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][
             22
         ]
+
+    def easy_asserts(self) -> None:
+        assert {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        } == expected, "Not what we expected"
+
+        assert expected == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }, "Not what we expected"
+
+        assert expected == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }
+
+    def tricky_asserts(self) -> None:
+        assert {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        } == expected(
+            value, is_going_to_be="too long to fit in a single line", srsly=True
+        ), "Not what we expected"
+
+        assert {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        } == expected, (
+            "Not what we expected and the message is too long to fit in one line"
+        )
+
+        assert expected(
+            value, is_going_to_be="too long to fit in a single line", srsly=True
+        ) == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }, "Not what we expected"
+
+        assert expected == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }, (
+            "Not what we expected and the message is too long to fit "
+            "in one line because it's too long"
+        )
+
+        # This is weird but true.
+        assert expectedexpectedexpectedexpectedexpectedexpectedexpectedexpectedexpect == {
+            key1: value1,
+            key2: value2,
+            key3: value3,
+            key4: value4,
+            key5: value5,
+            key6: value6,
+            key7: value7,
+            key8: value8,
+            key9: value9,
+        }

tests/empty_lines.py

@@ -123,29 +123,23 @@ def f():
             return NO
 
         if prevp.type == token.EQUAL:
-            if (
-                prevp.parent
-                and prevp.parent.type in {
-                    syms.typedargslist,
-                    syms.varargslist,
-                    syms.parameters,
-                    syms.arglist,
-                    syms.argument,
-                }
-            ):
+            if prevp.parent and prevp.parent.type in {
+                syms.typedargslist,
+                syms.varargslist,
+                syms.parameters,
+                syms.arglist,
+                syms.argument,
+            }:
                 return NO
 
         elif prevp.type == token.DOUBLESTAR:
-            if (
-                prevp.parent
-                and prevp.parent.type in {
-                    syms.typedargslist,
-                    syms.varargslist,
-                    syms.parameters,
-                    syms.arglist,
-                    syms.dictsetmaker,
-                }
-            ):
+            if prevp.parent and prevp.parent.type in {
+                syms.typedargslist,
+                syms.varargslist,
+                syms.parameters,
+                syms.arglist,
+                syms.dictsetmaker,
+            }:
                 return NO
 
 
@@ -183,14 +177,11 @@ def g():
             return NO
 
         if prevp.type == token.EQUAL:
-            if (
-                prevp.parent
-                and prevp.parent.type in {
-                    syms.typedargslist,
-                    syms.varargslist,
-                    syms.parameters,
-                    syms.arglist,
-                    syms.argument,
-                }
-            ):
+            if prevp.parent and prevp.parent.type in {
+                syms.typedargslist,
+                syms.varargslist,
+                syms.parameters,
+                syms.arglist,
+                syms.argument,
+            }:
                 return NO

tests/expression.diff

@@ -167,11 +167,11 @@
 -what_is_up_with_those_new_coord_names = (coord_names + set(vars_to_create)) + set(vars_to_remove)
 -what_is_up_with_those_new_coord_names = (coord_names | set(vars_to_create)) - set(vars_to_remove)
 -result = session.query(models.Customer.id).filter(models.Customer.account_id == account_id, models.Customer.email == email_address).order_by(models.Customer.id.asc(),).all()
-+what_is_up_with_those_new_coord_names = (
-+    (coord_names + set(vars_to_create)) + set(vars_to_remove)
++what_is_up_with_those_new_coord_names = (coord_names + set(vars_to_create)) + set(
++    vars_to_remove
 +)
-+what_is_up_with_those_new_coord_names = (
-+    (coord_names | set(vars_to_create)) - set(vars_to_remove)
++what_is_up_with_those_new_coord_names = (coord_names | set(vars_to_create)) - set(
++    vars_to_remove
 +)
 +result = session.query(models.Customer.id).filter(
 +    models.Customer.account_id == account_id, models.Customer.email == email_address
@@ -256,8 +256,8 @@
 -    ~ aaaaaaaaaaaaaaaa.a + aaaaaaaaaaaaaaaa.b - aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e | aaaaaaaaaaaaaaaa.f & aaaaaaaaaaaaaaaa.g % aaaaaaaaaaaaaaaa.h ^ aaaaaaaaaaaaaaaa.i << aaaaaaaaaaaaaaaa.k >> aaaaaaaaaaaaaaaa.l ** aaaaaaaaaaaaaaaa.m // aaaaaaaaaaaaaaaa.n
 +print(*lambda x: x)
 +assert not Test, "Short message"
-+assert (
-+    this is ComplexTest and not requirements.fit_in_a_single_line(force=False)
++assert this is ComplexTest and not requirements.fit_in_a_single_line(
++    force=False
 +), "Short message"
 +assert parens is TooMany
 +for (x,) in (1,), (2,), (3,):

tests/expression.py

@@ -401,11 +401,11 @@ async def f():
 c = 1
 d = (1,) + a + (2,)
 e = (1,).count(1)
-what_is_up_with_those_new_coord_names = (
-    (coord_names + set(vars_to_create)) + set(vars_to_remove)
+what_is_up_with_those_new_coord_names = (coord_names + set(vars_to_create)) + set(
+    vars_to_remove
 )
-what_is_up_with_those_new_coord_names = (
-    (coord_names | set(vars_to_create)) - set(vars_to_remove)
+what_is_up_with_those_new_coord_names = (coord_names | set(vars_to_create)) - set(
+    vars_to_remove
 )
 result = session.query(models.Customer.id).filter(
     models.Customer.account_id == account_id, models.Customer.email == email_address
@@ -433,8 +433,8 @@ async def f():
 print(**{1: 3} if False else {x: x for x in range(3)})
 print(*lambda x: x)
 assert not Test, "Short message"
-assert (
-    this is ComplexTest and not requirements.fit_in_a_single_line(force=False)
+assert this is ComplexTest and not requirements.fit_in_a_single_line(
+    force=False
 ), "Short message"
 assert parens is TooMany
 for (x,) in (1,), (2,), (3,):