Improve comprehension line break beheavior #5680

MichaReiser · 2023-07-11T09:57:53Z

Summary

This PR improves the Black compatibility when it comes to breaking comprehensions.

We want to avoid line breaks before the target and in whenever possible. Furthermore, if X is not None should be grouped together, similar to other binary like expressions

Test Plan

cargo test

MichaReiser · 2023-07-11T09:58:19Z

Current dependencies on/for this PR:

main
- PR Format target: annotation = value? expressions #5661
  - PR Improve comprehension line break beheavior #5680 👈
    - PR Only omit optinal parens if the expression ends or starts with a parenthesized expression #5683

This comment was auto-generated by Graphite.

MichaReiser · 2023-07-11T09:59:10Z

crates/ruff_python_formatter/src/expression/expr_compare.rs

@@ -33,36 +33,40 @@ impl FormatNodeRule<ExprCompare> for FormatExprCompare {

        let comments = f.context().comments().clone();

-        write!(f, [in_parentheses_only_group(&left.format())])?;
+        let inner = format_with(|f| {


No change other than that this gets all wrapped in an in_parentheses_only_group

MichaReiser · 2023-07-11T10:00:57Z

crates/ruff_python_formatter/tests/snapshots/format@statement__list_comp.py.snap

+    for (
+        ccccccccccccccccccccccccccccccccccccccc,
+        ddddddddddddddddddd,
+        [eeeeeeeeeeeeeeeeeeeeee, fffffffffffffffffffffffff],
+    ) in eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeffffffffffffffffffffffffggggggggggggggggggggghhhhhhhhhhhhhhothermoreeand_even_moreddddddddddddddddddddd


Black doesn't insert parentheses here. I'll look into this next

yep i think that black remove tuple parentheses here same as for statements. i'd expect this is the same case for if-else expressions being analogous to if statements

github-actions · 2023-07-11T10:33:00Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00     10.4±0.33ms     3.9 MB/sec    1.15     11.9±0.96ms     3.4 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.4±0.05ms     7.0 MB/sec    1.05      2.5±0.13ms     6.6 MB/sec
formatter/numpy/globals.py                 1.02   288.4±19.15µs    10.2 MB/sec    1.00   282.7±18.32µs    10.4 MB/sec
formatter/pydantic/types.py                1.07      5.7±0.50ms     4.4 MB/sec    1.00      5.4±0.31ms     4.7 MB/sec
linter/all-rules/large/dataset.py          1.03     19.4±1.23ms     2.1 MB/sec    1.00     18.7±0.71ms     2.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.5±0.15ms     3.7 MB/sec    1.01      4.5±0.17ms     3.7 MB/sec
linter/all-rules/numpy/globals.py          1.00   585.5±27.18µs     5.0 MB/sec    1.01   592.4±30.12µs     5.0 MB/sec
linter/all-rules/pydantic/types.py         1.01      8.2±0.28ms     3.1 MB/sec    1.00      8.1±0.35ms     3.2 MB/sec
linter/default-rules/large/dataset.py      1.00      9.0±0.20ms     4.5 MB/sec    1.06      9.5±0.47ms     4.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1942.6±49.28µs     8.6 MB/sec    1.17      2.3±0.34ms     7.3 MB/sec
linter/default-rules/numpy/globals.py      1.00   231.0±10.75µs    12.8 MB/sec    1.02   235.5±19.77µs    12.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.2±0.17ms     6.0 MB/sec    1.00      4.2±0.16ms     6.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.02      9.6±0.12ms     4.3 MB/sec    1.00      9.4±0.08ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.2±0.04ms     7.7 MB/sec    1.01      2.2±0.14ms     7.7 MB/sec
formatter/numpy/globals.py                 1.00    238.1±4.50µs    12.4 MB/sec    1.02    243.7±7.53µs    12.1 MB/sec
formatter/pydantic/types.py                1.01      4.7±0.07ms     5.4 MB/sec    1.00      4.7±0.08ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.10     18.2±0.23ms     2.2 MB/sec    1.00     16.5±0.24ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.10      4.6±0.11ms     3.6 MB/sec    1.00      4.2±0.07ms     3.9 MB/sec
linter/all-rules/numpy/globals.py          1.05   536.5±11.07µs     5.5 MB/sec    1.00    509.4±7.81µs     5.8 MB/sec
linter/all-rules/pydantic/types.py         1.12      8.1±0.18ms     3.2 MB/sec    1.00      7.2±0.12ms     3.5 MB/sec
linter/default-rules/large/dataset.py      1.24     10.2±0.15ms     4.0 MB/sec    1.00      8.2±0.08ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.18      2.1±0.03ms     8.1 MB/sec    1.00  1746.3±25.40µs     9.5 MB/sec
linter/default-rules/numpy/globals.py      1.12    227.1±4.37µs    13.0 MB/sec    1.00    202.4±5.45µs    14.6 MB/sec
linter/default-rules/pydantic/types.py     1.20      4.4±0.12ms     5.8 MB/sec    1.00      3.7±0.04ms     7.0 MB/sec

konstin · 2023-07-11T10:43:57Z

crates/ruff_python_formatter/src/other/comprehension.rs

                    target.format(),
-                    soft_line_break_or_space(),
+                    in_spacer,


would the macros rules allow inlining the in_space as if expression?

I don't think so because space and soft_line_break_or_space return different structs.

konstin · 2023-07-11T10:44:18Z

crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_37__python37.py.snap

-            i
-            in
-            aitertools._async_map(self.async_inc, arange(8), batch_size=3)
+            async for i in aitertools._async_map(


that looks much nicer

konstin · 2023-07-11T10:44:45Z

crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__fmtskip5.py.snap

 a, b, c = 3, 4, 5
 if (
     a == 3
 -    and b    != 9  # fmt: skip
-+    and b
-+    != 9  # fmt: skip
+    and b != 9  # fmt: skip


konstin · 2023-07-11T10:46:12Z

crates/ruff_python_formatter/tests/snapshots/format@statement__list_comp.py.snap

+    for (
+        ccccccccccccccccccccccccccccccccccccccc,
+        ddddddddddddddddddd,
+        [eeeeeeeeeeeeeeeeeeeeee, fffffffffffffffffffffffff],
+    ) in eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeffffffffffffffffffffffffggggggggggggggggggggghhhhhhhhhhhhhhothermoreeand_even_moreddddddddddddddddddddd


yep i think that black remove tuple parentheses here same as for statements. i'd expect this is the same case for if-else expressions being analogous to if statements

MichaReiser · 2023-07-11T14:51:27Z

@MichaReiser merged this pull request with Graphite.

MichaReiser commented Jul 11, 2023

View reviewed changes

MichaReiser added the formatter Related to the formatter label Jul 11, 2023

MichaReiser requested a review from konstin July 11, 2023 10:01

MichaReiser force-pushed the comprehension-line-break branch from 71a4aa2 to 7357591 Compare July 11, 2023 10:02

MichaReiser linked an issue Jul 11, 2023 that may be closed by this pull request

Formatter: Don't insert a newline between if and parentheses in list comprehensions #5672

Closed

konstin approved these changes Jul 11, 2023

View reviewed changes

MichaReiser mentioned this pull request Jul 11, 2023

Only omit optinal parens if the expression ends or starts with a parenthesized expression #5683

Merged

MichaReiser force-pushed the format-annotated-assignment branch from a9c9dd5 to 3fbd025 Compare July 11, 2023 11:30

MichaReiser force-pushed the comprehension-line-break branch from 7357591 to 3bce23a Compare July 11, 2023 11:30

MichaReiser force-pushed the format-annotated-assignment branch from 3fbd025 to 30e7756 Compare July 11, 2023 12:31

MichaReiser force-pushed the comprehension-line-break branch from 3bce23a to e83b302 Compare July 11, 2023 12:32

Base automatically changed from format-annotated-assignment to main July 11, 2023 14:40

Improve comprehension line break beheavior

6e5dc7b

MichaReiser force-pushed the comprehension-line-break branch from e83b302 to 6e5dc7b Compare July 11, 2023 14:41

MichaReiser merged commit 8b9193a into main Jul 11, 2023

MichaReiser deleted the comprehension-line-break branch July 11, 2023 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve comprehension line break beheavior #5680

Improve comprehension line break beheavior #5680

MichaReiser commented Jul 11, 2023

MichaReiser commented Jul 11, 2023 •

edited

Loading

MichaReiser Jul 11, 2023

MichaReiser Jul 11, 2023

konstin Jul 11, 2023

github-actions bot commented Jul 11, 2023 •

edited

Loading

konstin Jul 11, 2023

MichaReiser Jul 11, 2023

konstin Jul 11, 2023

konstin Jul 11, 2023

konstin Jul 11, 2023

MichaReiser commented Jul 11, 2023

Improve comprehension line break beheavior #5680

Improve comprehension line break beheavior #5680

Conversation

MichaReiser commented Jul 11, 2023

Summary

Test Plan

MichaReiser commented Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 11, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Jul 11, 2023

MichaReiser commented Jul 11, 2023 •

edited

Loading

github-actions bot commented Jul 11, 2023 •

edited

Loading