Format `PatternMatchSequence` #6676

harupy · 2023-08-18T14:39:20Z

Summary

Test Plan

Do I need to add extra tests?

harupy · 2023-08-18T14:42:52Z

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs

+                .nodes(patterns.iter())
+                .finish()
+        });
+        parenthesized("[", &items, "]")


Oh case (...) is also PatternMatchSequence. I need to fix this.

MichaReiser · 2023-08-18T15:00:17Z

Do I need to add extra tests?

You can add additional tests to crates/ruff_python_formatter/resources/test/fixtures/ruff.

We like to add additional tests for handling comments because they tend to be tricky and black's test suite only has very few cases for it.

github-actions · 2023-08-18T15:11:51Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.0±0.03ms    10.2 MB/sec    1.00      4.0±0.02ms    10.2 MB/sec
formatter/numpy/ctypeslib.py               1.00    844.8±3.91µs    19.7 MB/sec    1.01    851.4±8.66µs    19.6 MB/sec
formatter/numpy/globals.py                 1.00     90.8±0.63µs    32.5 MB/sec    1.00     91.2±0.51µs    32.4 MB/sec
formatter/pydantic/types.py                1.00   1651.5±9.16µs    15.4 MB/sec    1.00  1657.8±26.39µs    15.4 MB/sec
linter/all-rules/large/dataset.py          1.00     12.0±0.04ms     3.4 MB/sec    1.00     12.1±0.05ms     3.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.02ms     5.1 MB/sec    1.00      3.3±0.01ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.03    457.4±2.07µs     6.5 MB/sec    1.00    444.9±8.05µs     6.6 MB/sec
linter/all-rules/pydantic/types.py         1.02      6.4±0.23ms     4.0 MB/sec    1.00      6.3±0.08ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.03      6.5±0.02ms     6.3 MB/sec    1.00      6.3±0.09ms     6.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1443.0±4.78µs    11.5 MB/sec    1.00  1429.7±12.72µs    11.6 MB/sec
linter/default-rules/numpy/globals.py      1.00    165.2±0.43µs    17.9 MB/sec    1.00    165.9±0.47µs    17.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.9±0.01ms     8.7 MB/sec    1.00      3.0±0.01ms     8.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.7±0.07ms    11.0 MB/sec    1.00      3.7±0.06ms    10.9 MB/sec
formatter/numpy/ctypeslib.py               1.01   763.0±11.59µs    21.8 MB/sec    1.00   759.0±10.86µs    21.9 MB/sec
formatter/numpy/globals.py                 1.00     79.0±2.04µs    37.3 MB/sec    1.01     79.7±1.39µs    37.0 MB/sec
formatter/pydantic/types.py                1.00  1528.3±27.20µs    16.7 MB/sec    1.01  1542.0±53.78µs    16.5 MB/sec
linter/all-rules/large/dataset.py          1.00     12.5±0.18ms     3.3 MB/sec    1.02     12.7±0.44ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.05ms     4.8 MB/sec    1.02      3.5±0.07ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    431.8±4.83µs     6.8 MB/sec    1.01    435.6±5.82µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.5±0.06ms     3.9 MB/sec    1.02      6.6±0.11ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.00      7.0±0.10ms     5.8 MB/sec    1.02      7.2±0.13ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1490.7±24.44µs    11.2 MB/sec    1.02  1526.4±29.92µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    169.1±2.52µs    17.5 MB/sec    1.03    173.4±3.42µs    17.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.05ms     8.1 MB/sec    1.03      3.2±0.06ms     7.9 MB/sec

harupy · 2023-08-19T04:09:17Z

@MichaReiser Got it, I will add tests

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs

konstin · 2023-08-21T12:59:48Z

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs

+            }
+            SequenceType::TupleWithoutParentheses => {
+                let items = format_with(|f| {
+                    f.join_with(&format_args![text(","), space()])


Please add a comment why you're not using join_comma_separated here

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs

harupy · 2023-08-21T13:52:49Z

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py

@@ -141,3 +141,26 @@ def foo():
    no_comments
    ):
        pass
+
+match foo:
+    case "a", "b":


With join_comma_separated, this is formatted to:

case "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue",:

which is a syntax error.

Should this be formatted like this?

case "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue":

or

case ( "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", ): pass

?

not sure tbh. black does not add parentheses even if the line is too long, but i also think it would be good if we would break instead of writing an overlong lines. Also black's behavior is inconsistent with other overlong tuples, e.g.

match x: case "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", "aslödjhkfö": pass b = "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", "aslödjhkfö"

becomes

match x: case "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", "aslödjhkfö": pass b = ( "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", "aslödjhkfö", )

CC @MichaReiser what do you think?

I'm not well at the moment and this is taking more brain capacity than I have right now. I hope to get back to this tomorrow

i also think it would be good if we would break instead of writing an overlong lines.

+1

The sequence pattern is a bit strange IMO because matching [a, b] or (a, b) or a, b are semantically identical. Meaning, the sequence is neither a list nor a tuple... it's its own thing.

But I agree, I would expect the sequences to break the same as tuples (and lists) do

psf/black#3726 and psf/black#3403 seem related.

harupy · 2023-08-21T14:08:58Z

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs

+        });
+        match sequence_type {
+            SequenceType::Tuple | SequenceType::TupleWithoutParentheses => {
+                parenthesized("(", &items, ")")


This is still not correct because parenthesized always wraps the sequence with ().

charliermarsh · 2023-08-23T00:36:52Z

Did a quick read -- gonna merge to keep things moving along. Can always address follow-ups in new PRs.

harupy commented Aug 18, 2023

View reviewed changes

harupy commented Aug 19, 2023

View reviewed changes

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py Outdated Show resolved Hide resolved

harupy added 2 commits August 21, 2023 16:22

Format FormatPatternSequence

b9514b3

tests

572975e

konstin added the formatter Related to the formatter label Aug 21, 2023

wip

16fba95

harupy force-pushed the format-FormatPatternSequence branch from e9e5268 to 16fba95 Compare August 21, 2023 09:40

harupy added 2 commits August 21, 2023 19:41

wip

99ef84a

wip

9a646ab

konstin approved these changes Aug 21, 2023

View reviewed changes

harupy added 3 commits August 21, 2023 22:16

Add test case

62acefd

Refactor

34040f8

Comme on why join_comma_separated is not used

20b9630

harupy commented Aug 21, 2023

View reviewed changes

crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs Outdated Show resolved Hide resolved

Add test case for trailing comma

8d9b117

harupy commented Aug 21, 2023

View reviewed changes

harupy added 2 commits August 21, 2023 23:01

Use join_comma_separated

e6acdbc

Remove unused imports

1c00557

harupy commented Aug 21, 2023

View reviewed changes

harupy added 4 commits August 22, 2023 16:54

merge

36f3cba

fix

0203a8d

Remove .snap.new files

f5946d6

Optionally break the sequence as tuple

74967e8

harupy requested review from MichaReiser and konstin August 22, 2023 09:31

konstin approved these changes Aug 22, 2023

View reviewed changes

Merge branch 'main' into format-FormatPatternSequence

da75811

charliermarsh enabled auto-merge (squash) August 23, 2023 00:35

charliermarsh merged commit 94f5f18 into astral-sh:main Aug 23, 2023
16 checks passed

harupy deleted the format-FormatPatternSequence branch August 23, 2023 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format `PatternMatchSequence` #6676

Format `PatternMatchSequence` #6676

harupy commented Aug 18, 2023

harupy Aug 18, 2023 •

edited

Loading

MichaReiser commented Aug 18, 2023

github-actions bot commented Aug 18, 2023 •

edited

Loading

harupy commented Aug 19, 2023

konstin Aug 21, 2023

harupy Aug 21, 2023

harupy Aug 21, 2023 •

edited

Loading

harupy Aug 21, 2023

konstin Aug 21, 2023

MichaReiser Aug 21, 2023

harupy Aug 21, 2023

MichaReiser Aug 22, 2023

harupy Aug 22, 2023 •

edited

Loading

harupy Aug 21, 2023 •

edited

Loading

charliermarsh commented Aug 23, 2023

Format PatternMatchSequence #6676

Format PatternMatchSequence #6676

Conversation

harupy commented Aug 18, 2023

Summary

Test Plan

harupy Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

MichaReiser commented Aug 18, 2023

github-actions bot commented Aug 18, 2023 • edited Loading

PR Check Results

Benchmark

Linux

Windows

harupy commented Aug 19, 2023

konstin Aug 21, 2023

Choose a reason for hiding this comment

harupy Aug 21, 2023

Choose a reason for hiding this comment

harupy Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

harupy Aug 21, 2023

Choose a reason for hiding this comment

konstin Aug 21, 2023

Choose a reason for hiding this comment

MichaReiser Aug 21, 2023

Choose a reason for hiding this comment

harupy Aug 21, 2023

Choose a reason for hiding this comment

MichaReiser Aug 22, 2023

Choose a reason for hiding this comment

harupy Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

harupy Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh commented Aug 23, 2023

Format `PatternMatchSequence` #6676

Format `PatternMatchSequence` #6676

harupy Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading

harupy Aug 21, 2023 •

edited

Loading

harupy Aug 22, 2023 •

edited

Loading

harupy Aug 21, 2023 •

edited

Loading