From 7e2eba25926c73f2a585d2ad7e746300b4c3a7d3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 16 Sep 2023 13:19:34 -0400 Subject: [PATCH] Avoiding grouping comprehension targets separately from conditions (#7429) ## Summary Black seems to treat comprehension targets and conditions as if they're in a single group -- so if the comprehension expands, all conditions are rendered on their own line, etc. Closes https://github.com/astral-sh/ruff/issues/7421. ## Test Plan `cargo test` No change in any of the similarity metrics. Before: | project | similarity index | total files | changed files | |--------------|------------------:|------------------:|------------------:| | cpython | 0.76083 | 1789 | 1632 | | django | 0.99982 | 2760 | 37 | | transformers | 0.99957 | 2587 | 399 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99983 | 3496 | 18 | | warehouse | 0.99923 | 648 | 18 | | zulip | 0.99962 | 1437 | 22 | After: | project | similarity index | total files | changed files | |--------------|------------------:|------------------:|------------------:| | cpython | 0.76083 | 1789 | 1632 | | django | 0.99982 | 2760 | 37 | | transformers | 0.99957 | 2587 | 399 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99983 | 3496 | 18 | | warehouse | 0.99923 | 648 | 18 | | zulip | 0.99962 | 1437 | 22 | --- .../fixtures/ruff/expression/list_comp.py | 22 ++++++++ .../src/other/comprehension.rs | 24 ++++---- .../format@expression__list_comp.py.snap | 56 +++++++++++++++++++ 3 files changed, 89 insertions(+), 13 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py index 21f89cb019..f2d8757c28 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/list_comp.py @@ -73,3 +73,25 @@ a = [ aaaaaaaaaaaaaaaaaaaaa = [ o for o in self.registry.values if o.__class__ is not ModelAdmin ] + +[ + task + for head_name in head_nodes + if (task := self.get_task_by_name(head_name)) + if not None +] + +[ + f(x, y,) + for head_name in head_nodes if head_name +] + +[ + f(x, y) + for head_name in f(x, y,) if head_name +] + +[ + x + for (x, y,) in z if head_name +] diff --git a/crates/ruff_python_formatter/src/other/comprehension.rs b/crates/ruff_python_formatter/src/other/comprehension.rs index d5f439a676..788e782ab7 100644 --- a/crates/ruff_python_formatter/src/other/comprehension.rs +++ b/crates/ruff_python_formatter/src/other/comprehension.rs @@ -56,16 +56,14 @@ impl FormatNodeRule for FormatComprehension { [ token("for"), trailing_comments(before_target_comments), - group(&format_args!( - Spacer(target), - ExprTupleWithoutParentheses(target), - in_spacer, - leading_comments(before_in_comments), - token("in"), - trailing_comments(trailing_in_comments), - Spacer(iter), - iter.format(), - )), + Spacer(target), + ExprTupleWithoutParentheses(target), + in_spacer, + leading_comments(before_in_comments), + token("in"), + trailing_comments(trailing_in_comments), + Spacer(iter), + iter.format(), ] )?; if !ifs.is_empty() { @@ -79,18 +77,18 @@ impl FormatNodeRule for FormatComprehension { dangling_if_comments .partition_point(|comment| comment.line_position().is_own_line()), ); - joiner.entry(&group(&format_args!( + joiner.entry(&format_args!( leading_comments(own_line_if_comments), token("if"), trailing_comments(end_of_line_if_comments), Spacer(if_case), if_case.format(), - ))); + )); } joiner.finish() }); - write!(f, [soft_line_break_or_space(), group(&joined)])?; + write!(f, [soft_line_break_or_space(), joined])?; } Ok(()) } diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap index 57e59f3c04..b3a09e301c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__list_comp.py.snap @@ -79,6 +79,28 @@ a = [ aaaaaaaaaaaaaaaaaaaaa = [ o for o in self.registry.values if o.__class__ is not ModelAdmin ] + +[ + task + for head_name in head_nodes + if (task := self.get_task_by_name(head_name)) + if not None +] + +[ + f(x, y,) + for head_name in head_nodes if head_name +] + +[ + f(x, y) + for head_name in f(x, y,) if head_name +] + +[ + x + for (x, y,) in z if head_name +] ``` ## Output @@ -178,6 +200,40 @@ a = [ aaaaaaaaaaaaaaaaaaaaa = [ o for o in self.registry.values if o.__class__ is not ModelAdmin ] + +[ + task + for head_name in head_nodes + if (task := self.get_task_by_name(head_name)) + if not None +] + +[ + f( + x, + y, + ) + for head_name in head_nodes + if head_name +] + +[ + f(x, y) + for head_name in f( + x, + y, + ) + if head_name +] + +[ + x + for ( + x, + y, + ) in z + if head_name +] ```