From ee44f3009a501932692ac9c167bb6c1a31f745be Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Thu, 21 Oct 2021 00:28:23 -0700 Subject: [PATCH] Remove magic comma logic from can_omit_invisible_parens It was causing stability issues because the first pass could cause a "magic trailing comma" to appear, meaning that the second pass might get a different result. It's not critical. black-primer previously crashed on several projects due to instability, but now passes. The preexisting disabled tests can now be enabled. Added a new test for a case that came up in my testing. Some things format differently (with extra parens) --- CHANGES.md | 1 + src/black/linegen.py | 2 +- src/black/lines.py | 14 +---------- src/black/nodes.py | 23 ------------------- tests/data/function_trailing_comma.py | 23 +++++++++++-------- tests/data/long_strings_flag_disabled.py | 13 +++++++---- tests/data/trailing_comma_optional_parens1.py | 7 +++++- tests/test_black.py | 5 +--- 8 files changed, 31 insertions(+), 57 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 76a7ca6fd3d..ed396fbadc7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,7 @@ - Add new `--workers` parameter (#2514) - Fixed feature detection for positional-only arguments in lambdas (#2532) - Bumped typed-ast version minimum to 1.4.3 for 3.10 compatiblity (#2519) +- Fix unstable black runs around magic trailing comma (#2572) ### _Blackd_ diff --git a/src/black/linegen.py b/src/black/linegen.py index eb53fa0ac56..aa2b8205421 100644 --- a/src/black/linegen.py +++ b/src/black/linegen.py @@ -496,7 +496,7 @@ def right_hand_split( # there are no standalone comments in the body and not body.contains_standalone_comments(0) # and we can actually remove the parens - and can_omit_invisible_parens(body, line_length, omit_on_explode=omit) + and can_omit_invisible_parens(body, line_length) ): omit = {id(closing_bracket), *omit} try: diff --git a/src/black/lines.py b/src/black/lines.py index 63225c0e6d3..52be2a18b82 100644 --- a/src/black/lines.py +++ b/src/black/lines.py @@ -3,7 +3,6 @@ import sys from typing import ( Callable, - Collection, Dict, Iterator, List, @@ -22,7 +21,7 @@ from black.nodes import STANDALONE_COMMENT, TEST_DESCENDANTS from black.nodes import BRACKETS, OPENING_BRACKETS, CLOSING_BRACKETS from black.nodes import syms, whitespace, replace_child, child_towards -from black.nodes import is_multiline_string, is_import, is_type_comment, last_two_except +from black.nodes import is_multiline_string, is_import, is_type_comment from black.nodes import is_one_tuple_between # types @@ -609,7 +608,6 @@ def can_be_split(line: Line) -> bool: def can_omit_invisible_parens( line: Line, line_length: int, - omit_on_explode: Collection[LeafID] = (), ) -> bool: """Does `line` have a shape safe to reformat without optional parens around it? @@ -647,12 +645,6 @@ def can_omit_invisible_parens( penultimate = line.leaves[-2] last = line.leaves[-1] - if line.magic_trailing_comma: - try: - penultimate, last = last_two_except(line.leaves, omit=omit_on_explode) - except LookupError: - # Turns out we'd omit everything. We cannot skip the optional parentheses. - return False if ( last.type == token.RPAR @@ -674,10 +666,6 @@ def can_omit_invisible_parens( # unnecessary. return True - if line.magic_trailing_comma and penultimate.type == token.COMMA: - # The rightmost non-omitted bracket pair is the one we want to explode on. - return True - if _can_omit_closing_paren(line, last=last, line_length=line_length): return True diff --git a/src/black/nodes.py b/src/black/nodes.py index 8f2e15b2cc3..0f84ad5800b 100644 --- a/src/black/nodes.py +++ b/src/black/nodes.py @@ -4,13 +4,11 @@ import sys from typing import ( - Collection, Generic, Iterator, List, Optional, Set, - Tuple, TypeVar, Union, ) @@ -439,27 +437,6 @@ def prev_siblings_are(node: Optional[LN], tokens: List[Optional[NodeType]]) -> b return prev_siblings_are(node.prev_sibling, tokens[:-1]) -def last_two_except(leaves: List[Leaf], omit: Collection[LeafID]) -> Tuple[Leaf, Leaf]: - """Return (penultimate, last) leaves skipping brackets in `omit` and contents.""" - stop_after = None - last = None - for leaf in reversed(leaves): - if stop_after: - if leaf is stop_after: - stop_after = None - continue - - if last: - return leaf, last - - if id(leaf) in omit: - stop_after = leaf.opening_bracket - else: - last = leaf - else: - raise LookupError("Last two leaves were also skipped") - - def parent_type(node: Optional[LN]) -> Optional[NodeType]: """ Returns: diff --git a/tests/data/function_trailing_comma.py b/tests/data/function_trailing_comma.py index 02078219e82..429eb0e330f 100644 --- a/tests/data/function_trailing_comma.py +++ b/tests/data/function_trailing_comma.py @@ -89,16 +89,19 @@ def f( "a": 1, "b": 2, }["a"] - if a == { - "a": 1, - "b": 2, - "c": 3, - "d": 4, - "e": 5, - "f": 6, - "g": 7, - "h": 8, - }["a"]: + if ( + a + == { + "a": 1, + "b": 2, + "c": 3, + "d": 4, + "e": 5, + "f": 6, + "g": 7, + "h": 8, + }["a"] + ): pass diff --git a/tests/data/long_strings_flag_disabled.py b/tests/data/long_strings_flag_disabled.py index ef3094fd779..db3954e3abd 100644 --- a/tests/data/long_strings_flag_disabled.py +++ b/tests/data/long_strings_flag_disabled.py @@ -133,11 +133,14 @@ "Use f-strings instead!", ) -old_fmt_string3 = "Whereas only the strings after the percent sign were long in the last example, this example uses a long initial string as well. This is another %s %s %s %s" % ( - "really really really really really", - "old", - "way to format strings!", - "Use f-strings instead!", +old_fmt_string3 = ( + "Whereas only the strings after the percent sign were long in the last example, this example uses a long initial string as well. This is another %s %s %s %s" + % ( + "really really really really really", + "old", + "way to format strings!", + "Use f-strings instead!", + ) ) fstring = f"f-strings definitely make things more {difficult} than they need to be for {{black}}. But boy they sure are handy. The problem is that some lines will need to have the 'f' whereas others do not. This {line}, for example, needs one." diff --git a/tests/data/trailing_comma_optional_parens1.py b/tests/data/trailing_comma_optional_parens1.py index 5ad29a8affd..cfb92ad1049 100644 --- a/tests/data/trailing_comma_optional_parens1.py +++ b/tests/data/trailing_comma_optional_parens1.py @@ -1,3 +1,8 @@ if e1234123412341234.winerror not in (_winapi.ERROR_SEM_TIMEOUT, _winapi.ERROR_PIPE_BUSY) or _check_timeout(t): - pass \ No newline at end of file + pass + +if x: + if y: + new_id = max(Vegetable.objects.order_by('-id')[0].id, + Mineral.objects.order_by('-id')[0].id) + 1 diff --git a/tests/test_black.py b/tests/test_black.py index 1fc63c942e9..d159de53621 100644 --- a/tests/test_black.py +++ b/tests/test_black.py @@ -220,21 +220,18 @@ def _test_wip(self) -> None: black.assert_equivalent(source, actual) black.assert_stable(source, actual, black.FileMode()) - @unittest.expectedFailure @patch("black.dump_to_file", dump_to_stderr) - def test_trailing_comma_optional_parens_stability1(self) -> None: + def test_trailing_comma_optional_parens_stability11(self) -> None: source, _expected = read_data("trailing_comma_optional_parens1") actual = fs(source) black.assert_stable(source, actual, DEFAULT_MODE) - @unittest.expectedFailure @patch("black.dump_to_file", dump_to_stderr) def test_trailing_comma_optional_parens_stability2(self) -> None: source, _expected = read_data("trailing_comma_optional_parens2") actual = fs(source) black.assert_stable(source, actual, DEFAULT_MODE) - @unittest.expectedFailure @patch("black.dump_to_file", dump_to_stderr) def test_trailing_comma_optional_parens_stability3(self) -> None: source, _expected = read_data("trailing_comma_optional_parens3")