From 9854d4b375ef84d651892785165a6c7b0e9f391b Mon Sep 17 00:00:00 2001 From: Augie Fackler Date: Sun, 20 Oct 2019 10:08:34 -0400 Subject: [PATCH] Tweak collection literals to explode with trailing comma (#826) --- black.py | 129 +++++++++++++-------- tests/data/collections.py | 160 ++++++++++++++++++++++++++ tests/data/comments2.py | 4 +- tests/data/comments7.py | 2 +- tests/data/expression.diff | 31 +++-- tests/data/expression.py | 24 +++- tests/data/fmtonoff.py | 2 +- tests/data/function.py | 2 +- tests/data/function2.py | 2 +- tests/data/function_trailing_comma.py | 4 +- tests/test_black.py | 7 ++ 11 files changed, 299 insertions(+), 68 deletions(-) create mode 100644 tests/data/collections.py diff --git a/black.py b/black.py index 0776983..40b4d2b 100644 --- a/black.py +++ b/black.py @@ -1240,6 +1240,61 @@ class Line: Leaf(token.DOT, ".") for _ in range(3) ] + @property + def is_collection_with_optional_trailing_comma(self) -> bool: + """Is this line a collection literal with a trailing comma that's optional? + + Note that the trailing comma in a 1-tuple is not optional. + """ + if not self.leaves or len(self.leaves) < 4: + return False + # Look for and address a trailing colon. + if self.leaves[-1].type == token.COLON: + closer = self.leaves[-2] + close_index = -2 + else: + closer = self.leaves[-1] + close_index = -1 + if closer.type not in CLOSING_BRACKETS or self.inside_brackets: + return False + if closer.type == token.RPAR: + # Tuples require an extra check, because if there's only + # one element in the tuple removing the comma unmakes the + # tuple. + # + # We also check for parens before looking for the trailing + # comma because in some cases (eg assigning a dict + # literal) the literal gets wrapped in temporary parens + # during parsing. This case is covered by the + # collections.py test data. + opener = closer.opening_bracket + for _open_index, leaf in enumerate(self.leaves): + if leaf is opener: + break + else: + # Couldn't find the matching opening paren, play it safe. + return False + commas = 0 + comma_depth = self.leaves[close_index - 1].bracket_depth + for leaf in self.leaves[_open_index + 1 : close_index]: + if leaf.bracket_depth == comma_depth and leaf.type == token.COMMA: + commas += 1 + if commas > 1: + # We haven't looked yet for the trailing comma because + # we might also have caught noop parens. + return self.leaves[close_index - 1].type == token.COMMA + elif commas == 1: + return False # it's either a one-tuple or didn't have a trailing comma + if self.leaves[close_index - 1].type in CLOSING_BRACKETS: + close_index -= 1 + closer = self.leaves[close_index] + if closer.type == token.RPAR: + # TODO: this is a gut feeling. Will we ever see this? + return False + if self.leaves[close_index - 1].type != token.COMMA: + return False + return True + @property def is_def(self) -> bool: """Is this a function definition? (Also returns True for async defs.)""" @@ -1365,60 +1420,39 @@ class Line: def maybe_remove_trailing_comma(self, closing: Leaf) -> bool: """Remove trailing comma if there is one and it's safe.""" + if not (self.leaves and self.leaves[-1].type == token.COMMA): + return False + # We remove trailing commas only in the case of importing a + # single name from a module. if not ( self.leaves + and self.is_import + and len(self.leaves) > 4 and self.leaves[-1].type == token.COMMA and closing.type in CLOSING_BRACKETS + and self.leaves[-4].type == token.NAME + and ( + # regular `from foo import bar,` + self.leaves[-4].value == "import" + # `from foo import (bar as baz,) + or ( + len(self.leaves) > 6 + and self.leaves[-6].value == "import" + and self.leaves[-3].value == "as" + ) + # `from foo import bar as baz,` + or ( + len(self.leaves) > 5 + and self.leaves[-5].value == "import" + and self.leaves[-3].value == "as" + ) + ) + and closing.type == token.RPAR ): return False - if closing.type == token.RBRACE: - self.remove_trailing_comma() - return True - - if closing.type == token.RSQB: - comma = self.leaves[-1] - if comma.parent and comma.parent.type == syms.listmaker: - self.remove_trailing_comma() - return True - - # For parens let's check if it's safe to remove the comma. - # Imports are always safe. - if self.is_import: - self.remove_trailing_comma() - return True - - # Otherwise, if the trailing one is the only one, we might mistakenly - # change a tuple into a different type by removing the comma. - depth = closing.bracket_depth + 1 - commas = 0 - opening = closing.opening_bracket - for _opening_index, leaf in enumerate(self.leaves): - if leaf is opening: - break - - else: - return False - - for leaf in self.leaves[_opening_index + 1 :]: - if leaf is closing: - break - - bracket_depth = leaf.bracket_depth - if bracket_depth == depth and leaf.type == token.COMMA: - commas += 1 - if leaf.parent and leaf.parent.type in { - syms.arglist, - syms.typedargslist, - }: - commas += 1 - break - - if commas > 1: - self.remove_trailing_comma() - return True - - return False + self.remove_trailing_comma() + return True def append_comment(self, comment: Leaf) -> bool: """Add an inline or standalone comment to the line.""" @@ -2363,6 +2397,7 @@ def split_line( if ( not line.contains_uncollapsable_type_comments() and not line.should_explode + and not line.is_collection_with_optional_trailing_comma and ( is_line_short_enough(line, line_length=line_length, line_str=line_str) or line.contains_unsplittable_type_ignore() diff --git a/tests/data/collections.py b/tests/data/collections.py new file mode 100644 index 0000000..d408bf9 --- /dev/null +++ b/tests/data/collections.py @@ -0,0 +1,160 @@ +import core, time, a + +from . import A, B, C + +# unwraps +from foo import ( + bar, +) + +# stays wrapped +from foo import ( + baz, + qux, +) + +# as doesn't get confusing when unwrapped +from foo import ( + xyzzy as magic, +) + +a = {1,2,3,} +b = { +1,2, + 3} +c = { + 1, + 2, + 3, +} +x = 1, +y = narf(), +nested = {(1,2,3),(4,5,6),} +nested_no_trailing_comma = {(1,2,3),(4,5,6)} +nested_long_lines = ["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "cccccccccccccccccccccccccccccccccccccccc", (1, 2, 3), "dddddddddddddddddddddddddddddddddddddddd"] +{"oneple": (1,),} +{"oneple": (1,)} +['ls', 'lsoneple/%s' % (foo,)] +x = {"oneple": (1,)} +y = {"oneple": (1,),} +assert False, ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa wraps %s" % bar) + +# looping over a 1-tuple should also not get wrapped +for x in (1,): + pass +for (x,) in (1,), (2,), (3,): + pass + +[1, 2, 3,] + +division_result_tuple = (6/2,) +print("foo %r", (foo.bar,)) + +if True: + IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = ( + Config.IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING + | {pylons.controllers.WSGIController} + ) + +if True: + ec2client.get_waiter('instance_stopped').wait( + InstanceIds=[instance.id], + WaiterConfig={ + 'Delay': 5, + }) + ec2client.get_waiter("instance_stopped").wait( + InstanceIds=[instance.id], + WaiterConfig={"Delay": 5,}, + ) + ec2client.get_waiter("instance_stopped").wait( + InstanceIds=[instance.id], WaiterConfig={"Delay": 5,}, + ) + +# output + + +import core, time, a + +from . import A, B, C + +# unwraps +from foo import bar + +# stays wrapped +from foo import ( + baz, + qux, +) + +# as doesn't get confusing when unwrapped +from foo import xyzzy as magic + +a = { + 1, + 2, + 3, +} +b = {1, 2, 3} +c = { + 1, + 2, + 3, +} +x = (1,) +y = (narf(),) +nested = { + (1, 2, 3), + (4, 5, 6), +} +nested_no_trailing_comma = {(1, 2, 3), (4, 5, 6)} +nested_long_lines = [ + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "cccccccccccccccccccccccccccccccccccccccc", + (1, 2, 3), + "dddddddddddddddddddddddddddddddddddddddd", +] +{ + "oneple": (1,), +} +{"oneple": (1,)} +["ls", "lsoneple/%s" % (foo,)] +x = {"oneple": (1,)} +y = { + "oneple": (1,), +} +assert False, ( + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa wraps %s" + % bar +) + +# looping over a 1-tuple should also not get wrapped +for x in (1,): + pass +for (x,) in (1,), (2,), (3,): + pass + +[ + 1, + 2, + 3, +] + +division_result_tuple = (6 / 2,) +print("foo %r", (foo.bar,)) + +if True: + IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = Config.IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING | { + pylons.controllers.WSGIController + } + +if True: + ec2client.get_waiter("instance_stopped").wait( + InstanceIds=[instance.id], WaiterConfig={"Delay": 5,} + ) + ec2client.get_waiter("instance_stopped").wait( + InstanceIds=[instance.id], WaiterConfig={"Delay": 5,}, + ) + ec2client.get_waiter("instance_stopped").wait( + InstanceIds=[instance.id], WaiterConfig={"Delay": 5,}, + ) diff --git a/tests/data/comments2.py b/tests/data/comments2.py index e928f6d..489661e 100644 --- a/tests/data/comments2.py +++ b/tests/data/comments2.py @@ -63,7 +63,7 @@ def inline_comments_in_brackets_ruin_everything(): parameters.children = [ children[0], # (1 body, - children[-1], # )1 + children[-1] # )1 ] parameters.children = [ children[0], @@ -142,7 +142,7 @@ short syms.simple_stmt, [ Node(statement, result), - Leaf(token.NEWLINE, '\n'), # FIXME: \r\n? + Leaf(token.NEWLINE, '\n') # FIXME: \r\n? ], ) diff --git a/tests/data/comments7.py b/tests/data/comments7.py index 088dc99..948b3b0 100644 --- a/tests/data/comments7.py +++ b/tests/data/comments7.py @@ -94,7 +94,7 @@ result = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx def func(): c = call( - 0.0123, 0.0456, 0.0789, 0.0123, 0.0789, a[-1] # type: ignore + 0.0123, 0.0456, 0.0789, 0.0123, 0.0789, a[-1], # type: ignore ) # The type: ignore exception only applies to line length, not diff --git a/tests/data/expression.diff b/tests/data/expression.diff index eff98f9..da20094 100644 --- a/tests/data/expression.diff +++ b/tests/data/expression.diff @@ -11,7 +11,7 @@ True False 1 -@@ -29,63 +29,84 @@ +@@ -29,63 +29,96 @@ ~great +value -1 @@ -61,14 +61,26 @@ [] [1, 2, 3, 4, 5, 6, 7, 8, 9, (10 or A), (11 or B), (12 or C)] -[1, 2, 3,] -+[1, 2, 3] ++[ ++ 1, ++ 2, ++ 3, ++] [*a] [*range(10)] -[*a, 4, 5,] -[4, *a, 5,] -[this_is_a_very_long_variable_which_will_force_a_delimiter_split, element, another, *more] -+[*a, 4, 5] -+[4, *a, 5] ++[ ++ *a, ++ 4, ++ 5, ++] ++[ ++ 4, ++ *a, ++ 5, ++] +[ + this_is_a_very_long_variable_which_will_force_a_delimiter_split, + element, @@ -118,14 +130,16 @@ call(**self.screen_kwargs) call(b, **self.screen_kwargs) lukasz.langa.pl -@@ -94,23 +115,23 @@ +@@ -94,23 +127,25 @@ 1.0 .real ....__class__ list[str] dict[str, int] tuple[str, ...] -tuple[str, int, float, dict[str, int],] -+tuple[str, int, float, dict[str, int]] ++tuple[ ++ str, int, float, dict[str, int], ++] very_long_variable_name_filters: t.List[ t.Tuple[str, t.Union[str, t.List[t.Optional[str]]]], ] @@ -146,7 +160,7 @@ slice[0:1:2] slice[:] slice[:-1] -@@ -134,113 +155,171 @@ +@@ -134,113 +169,171 @@ numpy[-(c + 1) :, d] numpy[:, l[-2]] numpy[:, ::-1] @@ -199,7 +213,7 @@ + .filter( + models.Customer.account_id == account_id, models.Customer.email == email_address + ) -+ .order_by(models.Customer.id.asc()) ++ .order_by(models.Customer.id.asc(),) + .all() +) Ø = set() @@ -391,3 +405,4 @@ return True last_call() # standalone comment at ENDMARKER + diff --git a/tests/data/expression.py b/tests/data/expression.py index 3851249..c9b149f 100644 --- a/tests/data/expression.py +++ b/tests/data/expression.py @@ -314,11 +314,23 @@ str or None if (1 if True else 2) else str or bytes or None (1, 2, 3) [] [1, 2, 3, 4, 5, 6, 7, 8, 9, (10 or A), (11 or B), (12 or C)] -[1, 2, 3] +[ + 1, + 2, + 3, +] [*a] [*range(10)] -[*a, 4, 5] -[4, *a, 5] +[ + *a, + 4, + 5, +] +[ + 4, + *a, + 5, +] [ this_is_a_very_long_variable_which_will_force_a_delimiter_split, element, @@ -367,7 +379,9 @@ call.me(maybe) list[str] dict[str, int] tuple[str, ...] -tuple[str, int, float, dict[str, int]] +tuple[ + str, int, float, dict[str, int], +] very_long_variable_name_filters: t.List[ t.Tuple[str, t.Union[str, t.List[t.Optional[str]]]], ] @@ -445,7 +459,7 @@ result = ( .filter( models.Customer.account_id == account_id, models.Customer.email == email_address ) - .order_by(models.Customer.id.asc()) + .order_by(models.Customer.id.asc(),) .all() ) Ø = set() diff --git a/tests/data/fmtonoff.py b/tests/data/fmtonoff.py index 5f62af0..fff751e 100644 --- a/tests/data/fmtonoff.py +++ b/tests/data/fmtonoff.py @@ -150,7 +150,7 @@ def single_literal_yapf_disable(): BAZ = { (1, 2, 3, 4), (5, 6, 7, 8), - (9, 10, 11, 12), + (9, 10, 11, 12) } # yapf: disable cfg.rule( "Default", "address", diff --git a/tests/data/function.py b/tests/data/function.py index 4754588..51234a1 100644 --- a/tests/data/function.py +++ b/tests/data/function.py @@ -230,7 +230,7 @@ def trailing_comma(): } -def f(a, **kwargs) -> A: +def f(a, **kwargs,) -> A: return ( yield from A( very_long_argument_name1=very_long_value_for_the_argument, diff --git a/tests/data/function2.py b/tests/data/function2.py index 0c9da12..f57a3f5 100644 --- a/tests/data/function2.py +++ b/tests/data/function2.py @@ -24,7 +24,7 @@ def h(): # output -def f(a, **kwargs) -> A: +def f(a, **kwargs,) -> A: with cache_dir(): if something: result = CliRunner().invoke( diff --git a/tests/data/function_trailing_comma.py b/tests/data/function_trailing_comma.py index 29fd99b..f2594c6 100644 --- a/tests/data/function_trailing_comma.py +++ b/tests/data/function_trailing_comma.py @@ -6,9 +6,9 @@ def f(a:int=1,): # output -def f(a): +def f(a,): ... -def f(a: int = 1): +def f(a: int = 1,): ... diff --git a/tests/test_black.py b/tests/test_black.py index 107f77d..a92798d 100644 --- a/tests/test_black.py +++ b/tests/test_black.py @@ -1369,6 +1369,13 @@ class BlackTestCase(unittest.TestCase): self.assertIn(path, pyi_cache) self.assertNotIn(path, normal_cache) + def test_collections(self) -> None: + source, expected = read_data("collections") + actual = fs(source) + self.assertFormatEqual(expected, actual) + black.assert_equivalent(source, actual) + black.assert_stable(source, actual, black.FileMode()) + def test_pipe_force_py36(self) -> None: source, expected = read_data("force_py36") result = CliRunner().invoke( -- 2.39.5