Don't explode trailers that fit in a single line

README.md

@@ -566,6 +566,9 @@ More details can be found in [CONTRIBUTING](CONTRIBUTING.md).
   on Python 3.6+ only code and Python 2.7+ code with the `unicode_literals`
   future import (#188, #198, #199)
 
+* fixed trailers (content with brackets) being unnecessarily exploded
+  into their own lines after a dedented closing bracket
+
 * fixed an invalid trailing comma sometimes left in imports (#185)
 
 * fixed non-deterministic formatting when multiple pairs of removable parentheses

black.py

@@ -24,6 +24,7 @@
     List,
     Optional,
     Pattern,
+    Sequence,
     Set,
     Tuple,
     Type,
@@ -41,6 +42,7 @@
 from blib2to3.pgen2 import driver, token
 from blib2to3.pgen2.parse import ParseError
 
+
 __version__ = "18.4a6"
 DEFAULT_LINE_LENGTH = 88
 
@@ -1828,11 +1830,7 @@ def split_line(
         return
 
     line_str = str(line).strip("\n")
-    if (
-        len(line_str) <= line_length
-        and "\n" not in line_str  # multiline strings
-        and not line.contains_standalone_comments()
-    ):
+    if is_line_short_enough(line, line_length=line_length, line_str=line_str):
         yield line
         return
 
@@ -1841,10 +1839,22 @@ def split_line(
         split_funcs = [left_hand_split]
     elif line.is_import:
         split_funcs = [explode_split]
-    elif line.inside_brackets:
-        split_funcs = [delimiter_split, standalone_comment_split, right_hand_split]
     else:
-        split_funcs = [right_hand_split]
+
+        def rhs(line: Line, py36: bool = False) -> Iterator[Line]:
+            for omit in generate_trailers_to_omit(line, line_length):
+                lines = list(right_hand_split(line, py36, omit=omit))
+                if is_line_short_enough(lines[0], line_length=line_length):
+                    yield from lines
+                    return
+
+            # All splits failed, best effort split with no omits.
+            yield from right_hand_split(line, py36)
+
+        if line.inside_brackets:
+            split_funcs = [delimiter_split, standalone_comment_split, rhs]
+        else:
+            split_funcs = [rhs]
     for split_func in split_funcs:
         # We are accumulating lines in `result` because we might want to abort
         # mission and return the original line in the end, or attempt a different
@@ -1917,6 +1927,8 @@ def right_hand_split(
     """Split line into many lines, starting with the last matching bracket pair.
 
     If the split was by optional parentheses, attempt splitting without them, too.
+    `omit` is a collection of closing bracket IDs that shouldn't be considered for
+    this split.
     """
     head = Line(depth=line.depth)
     body = Line(depth=line.depth + 1, inside_brackets=True)
@@ -2446,6 +2458,67 @@ def is_python36(node: Node) -> bool:
     return False
 
 
+def generate_trailers_to_omit(line: Line, line_length: int) -> Iterator[Set[LeafID]]:
+    """Generate sets of closing bracket IDs that should be omitted in a RHS.
+
+    Brackets can be omitted if the entire trailer up to and including
+    a preceding closing bracket fits in one line.
+
+    Yielded sets are cumulative (contain results of previous yields, too).  First
+    set is empty.
+    """
+
+    omit: Set[LeafID] = set()
+    yield omit
+
+    length = 4 * line.depth
+    opening_bracket = None
+    closing_bracket = None
+    optional_brackets: Set[LeafID] = set()
+    inner_brackets: Set[LeafID] = set()
+    for index, leaf in enumerate_reversed(line.leaves):
+        length += len(leaf.prefix) + len(leaf.value)
+        if length > line_length:
+            break
+
+        comment: Optional[Leaf]
+        for comment in line.comments_after(leaf, index):
+            if "\n" in comment.prefix:
+                break  # Oops, standalone comment!
+
+            length += len(comment.value)
+        else:
+            comment = None
+        if comment is not None:
+            break  # There was a standalone comment, we can't continue.
+
+        optional_brackets.discard(id(leaf))
+        if opening_bracket:
+            if leaf is opening_bracket:
+                opening_bracket = None
+            elif leaf.type in CLOSING_BRACKETS:
+                inner_brackets.add(id(leaf))
+        elif leaf.type in CLOSING_BRACKETS:
+            if not leaf.value:
+                optional_brackets.add(id(opening_bracket))
+                continue
+
+            if index > 0 and line.leaves[index - 1].type in OPENING_BRACKETS:
+                # Empty brackets would fail a split so treat them as "inner"
+                # brackets (e.g. only add them to the `omit` set if another
+                # pair of brackets was good enough.
+                inner_brackets.add(id(leaf))
+                continue
+
+            opening_bracket = leaf.opening_bracket
+            if closing_bracket:
+                omit.add(id(closing_bracket))
+                omit.update(inner_brackets)
+                inner_brackets.clear()
+                yield omit
+            closing_bracket = leaf
+
+
 def get_future_imports(node: Node) -> Set[str]:
     """Return a set of __future__ imports in the file."""
     imports = set()
@@ -2723,6 +2796,28 @@ def sub_twice(regex: Pattern[str], replacement: str, original: str) -> str:
     return regex.sub(replacement, regex.sub(replacement, original))
 
 
+def enumerate_reversed(sequence: Sequence[T]) -> Iterator[Tuple[Index, T]]:
+    """Like `reversed(enumerate(sequence))` if that were possible."""
+    index = len(sequence) - 1
+    for element in reversed(sequence):
+        yield (index, element)
+        index -= 1
+
+
+def is_line_short_enough(line: Line, *, line_length: int, line_str: str = "") -> bool:
+    """Return True if `line` is no longer than `line_length`.
+
+    Uses the provided `line_str` rendering, if any, otherwise computes a new one.
+    """
+    if not line_str:
+        line_str = str(line).strip("\n")
+    return (
+        len(line_str) <= line_length
+        and "\n" not in line_str  # multiline strings
+        and not line.contains_standalone_comments()
+    )
+
+
 CACHE_DIR = Path(user_cache_dir("black", version=__version__))
 
 

docs/reference/reference_functions.rst

@@ -20,6 +20,8 @@ Assertions and checks
 
 .. autofunction:: black.is_import
 
+.. autofunction:: black.is_line_short_enough
+
 .. autofunction:: black.is_one_tuple
 
 .. autofunction:: black.is_python36
@@ -94,6 +96,8 @@ Utilities
 
 .. autofunction:: black.ensure_visible
 
+.. autofunction:: black.enumerate_reversed
+
 .. autofunction:: black.generate_comments
 
 .. autofunction:: black.make_comment

tests/comments4.py

@@ -63,14 +63,26 @@ def foo(list_a, list_b):
     results = (
         User.query.filter(User.foo == "bar").filter(  # Because foo.
             db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b))
-        ).filter(
-            User.xyz.is_(None)
-        )
+        ).filter(User.xyz.is_(None))
         # Another comment about the filtering on is_quux goes here.
         .filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(
             User.created_at.desc()
-        ).with_for_update(
-            key_share=True
-        ).all()
+        ).with_for_update(key_share=True).all()
     )
     return results
+
+
+def foo2(list_a, list_b):
+    # Standalone comment reasonably placed.
+    return User.query.filter(User.foo == "bar").filter(
+        db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b))
+    ).filter(User.xyz.is_(None))
+
+
+def foo3(list_a, list_b):
+    return (
+        # Standlone comment but weirdly placed.
+        User.query.filter(User.foo == "bar").filter(
+            db.or_(User.field_a.astext.in_(list_a), User.field_b.astext.in_(list_b))
+        ).filter(User.xyz.is_(None))
+    )

tests/composition.py

@@ -32,3 +32,14 @@ def test(self) -> None:
                         # Another
                     ):
                         print(i)
+
+    def omitting_trailers() -> None:
+        get_collection(
+            hey_this_is_a_very_long_call, it_has_funny_attributes, really=True
+        )[OneLevelIndex]
+        get_collection(
+            hey_this_is_a_very_long_call, it_has_funny_attributes, really=True
+        )[OneLevelIndex][TwoLevelIndex][ThreeLevelIndex][FourLevelIndex]
+        d[0][1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16][17][18][19][20][21][
+            22
+        ]

tests/expression.diff

@@ -11,7 +11,7 @@
  True
  False
  1
-@@ -29,62 +29,84 @@
+@@ -29,62 +29,82 @@
  ~great
  +value
  -1
@@ -30,9 +30,7 @@
 -foo = (lambda port_id, ignore_missing: {"port1": port1_resource, "port2": port2_resource}[port_id])
 +foo = lambda port_id, ignore_missing: {
 +    "port1": port1_resource, "port2": port2_resource
-+}[
-+    port_id
-+]
++}[port_id]
  1 if True else 2
  str or None if True else str or bytes or None
  (str or None) if True else (str or bytes or None)
@@ -117,7 +115,7 @@
  call(**self.screen_kwargs)
  call(b, **self.screen_kwargs)
  lukasz.langa.pl
-@@ -93,11 +115,11 @@
+@@ -93,11 +113,11 @@
  1.0 .real
  ....__class__
  list[str]
@@ -130,7 +128,7 @@
  ]
  slice[0]
  slice[0:1]
-@@ -124,103 +146,140 @@
+@@ -124,103 +144,138 @@
  numpy[-(c + 1) :, d]
  numpy[:, l[-2]]
  numpy[:, ::-1]
@@ -177,9 +175,7 @@
 +)
 +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()
++).order_by(models.Customer.id.asc()).all()
  Ø = set()
  authors.łukasz.say_thanks()
  mapping = {

tests/expression.py

@@ -270,9 +270,7 @@ async def f():
 manylambdas = lambda x=lambda y=lambda z=1: z: y(): x()
 foo = lambda port_id, ignore_missing: {
     "port1": port1_resource, "port2": port2_resource
-}[
-    port_id
-]
+}[port_id]
 1 if True else 2
 str or None if True else str or bytes or None
 (str or None) if True else (str or bytes or None)
@@ -411,9 +409,7 @@ async def f():
 )
 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()
+).order_by(models.Customer.id.asc()).all()
 Ø = set()
 authors.łukasz.say_thanks()
 mapping = {

tests/function.py

@@ -169,9 +169,7 @@ def spaces2(result=_core.Value(None)):
 def example(session):
     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()
+    ).order_by(models.Customer.id.asc()).all()
 
 
 def long_lines():

tests/function2.py

@@ -2,6 +2,11 @@ def f(
   a,
   **kwargs,
 ) -> A:
+    with cache_dir():
+        if something:
+            result = (
+                CliRunner().invoke(black.main, [str(src1), str(src2), "--diff", "--check"])
+            )
     return A(
         very_long_argument_name1=very_long_value_for_the_argument,
         very_long_argument_name2=very_long_value_for_the_argument,
@@ -11,6 +16,11 @@ def f(
 # output
 
 def f(a, **kwargs) -> A:
+    with cache_dir():
+        if something:
+            result = CliRunner().invoke(
+                black.main, [str(src1), str(src2), "--diff", "--check"]
+            )
     return A(
         very_long_argument_name1=very_long_value_for_the_argument,
         very_long_argument_name2=very_long_value_for_the_argument,