From 3eea3aad864b83af3b6c477c32f15eb821fe9341 Mon Sep 17 00:00:00 2001 From: =?utf8?q?=C5=81ukasz=20Langa?= Date: Mon, 14 May 2018 12:05:39 -0700 Subject: [PATCH] Don't explode trailers that fit in a single line --- README.md | 3 + black.py | 111 +++++++++++++++++++++++-- docs/reference/reference_functions.rst | 4 + tests/comments4.py | 24 ++++-- tests/composition.py | 11 +++ tests/expression.diff | 14 ++-- tests/expression.py | 8 +- tests/function.py | 4 +- tests/function2.py | 10 +++ 9 files changed, 157 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 3297f87..69d423a 100644 --- a/README.md +++ b/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 diff --git a/black.py b/black.py index 43b9bd1..7823ae0 100644 --- a/black.py +++ b/black.py @@ -24,6 +24,7 @@ from typing import ( List, Optional, Pattern, + Sequence, Set, Tuple, Type, @@ -41,6 +42,7 @@ from blib2to3 import pygram, pytree 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__)) diff --git a/docs/reference/reference_functions.rst b/docs/reference/reference_functions.rst index ede46a4..0360411 100644 --- a/docs/reference/reference_functions.rst +++ b/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 diff --git a/tests/comments4.py b/tests/comments4.py index 241b6ce..9529990 100644 --- a/tests/comments4.py +++ b/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)) + ) diff --git a/tests/composition.py b/tests/composition.py index 287888d..ea213cf 100644 --- a/tests/composition.py +++ b/tests/composition.py @@ -32,3 +32,14 @@ class C: # 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 + ] diff --git a/tests/expression.diff b/tests/expression.diff index 423cf57..835b7b0 100644 --- a/tests/expression.diff +++ b/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 = { diff --git a/tests/expression.py b/tests/expression.py index 17b77c4..7bea9a7 100644 --- a/tests/expression.py +++ b/tests/expression.py @@ -270,9 +270,7 @@ lambda a, b, c=True, *vararg, d=(v1 << 2), e="str", **kwargs: a + b 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 @@ what_is_up_with_those_new_coord_names = ( ) 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 = { diff --git a/tests/function.py b/tests/function.py index ea27ebb..4cfc945 100644 --- a/tests/function.py +++ b/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(): diff --git a/tests/function2.py b/tests/function2.py index 1b9d7b6..e262e05 100644 --- a/tests/function2.py +++ b/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, -- 2.39.5