From 5a47fd13cc4c9f43270dd12c37577244c1eabfcc Mon Sep 17 00:00:00 2001 From: =?utf8?q?=C5=81ukasz=20Langa?= Date: Tue, 15 May 2018 20:52:22 -0700 Subject: [PATCH] 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 #177 Fixes #193 --- README.md | 5 ++ black.py | 52 ++++++++++---------- tests/composition.py | 110 +++++++++++++++++++++++++++++++++++++++++- tests/empty_lines.py | 51 ++++++++------------ tests/expression.diff | 12 ++--- tests/expression.py | 12 ++--- 6 files changed, 174 insertions(+), 68 deletions(-) diff --git a/README.md b/README.md index 6c99fd3..1570348 100644 --- a/README.md +++ b/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 diff --git a/black.py b/black.py index a21b51a..e7a7aa8 100644 --- a/black.py +++ b/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 @@ class Line: 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 @@ class Line: 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) diff --git a/tests/composition.py b/tests/composition.py index ea213cf..c3dfd3d 100644 --- a/tests/composition.py +++ b/tests/composition.py @@ -33,7 +33,7 @@ class C: ): 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 @@ class C: 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, + } diff --git a/tests/empty_lines.py b/tests/empty_lines.py index 5b7ce92..4c03e43 100644 --- a/tests/empty_lines.py +++ b/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 diff --git a/tests/expression.diff b/tests/expression.diff index 835b7b0..5ad905f 100644 --- a/tests/expression.diff +++ b/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,): diff --git a/tests/expression.py b/tests/expression.py index 7bea9a7..65833c1 100644 --- a/tests/expression.py +++ b/tests/expression.py @@ -401,11 +401,11 @@ b = (1,) 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 @@ print(*[] or [1]) 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,): -- 2.39.2