Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Format PatternMatchSequence #6676

Merged
merged 16 commits into from
Aug 23, 2023
Merged

Format PatternMatchSequence #6676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b9514b3
Format FormatPatternSequence
harupy Aug 18, 2023
572975e
tests
harupy Aug 19, 2023
16fba95
wip
harupy Aug 21, 2023
99ef84a
wip
harupy Aug 21, 2023
9a646ab
wip
harupy Aug 21, 2023
62acefd
Add test case
harupy Aug 21, 2023
34040f8
Refactor
harupy Aug 21, 2023
20b9630
Comme on why join_comma_separated is not used
harupy Aug 21, 2023
8d9b117
Add test case for trailing comma
harupy Aug 21, 2023
e6acdbc
Use join_comma_separated
harupy Aug 21, 2023
1c00557
Remove unused imports
harupy Aug 21, 2023
36f3cba
merge
harupy Aug 22, 2023
0203a8d
fix
harupy Aug 22, 2023
f5946d6
Remove .snap.new files
harupy Aug 22, 2023
74967e8
Optionally break the sequence as tuple
harupy Aug 22, 2023
da75811
Merge branch 'main' into format-FormatPatternSequence
charliermarsh Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ def foo():
case (a as b) as c:
pass


match pattern_singleton:
case (
# leading 1
Expand All @@ -187,3 +188,30 @@ def foo():
...
case False:
...


match foo:
konstin marked this conversation as resolved.
Show resolved Hide resolved
case "a", "b":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With join_comma_separated, this is formatted to:

    case "NOT_YET_IMPLEMENTED_PatternMatchValue",
    "NOT_YET_IMPLEMENTED_PatternMatchValue",:

which is a syntax error.

Copy link
Contributor Author

@harupy harupy Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be formatted like this?

    case "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue":

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or

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

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

+1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@harupy harupy Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pass
case "a", "b",:
pass
case ("a", "b"):
pass
case ["a", "b"]:
pass
case (["a", "b"]):
pass


match foo:
case [ # leading
# leading
# leading
# leading
"a", # trailing
# trailing
# trailing
# trailing
"b",
]:
pass
52 changes: 43 additions & 9 deletions crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,53 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_formatter::prelude::format_with;
use ruff_formatter::{Format, FormatResult};
use ruff_python_ast::PatternMatchSequence;

use crate::{not_yet_implemented_custom_text, FormatNodeRule, PyFormatter};
use crate::builders::PyFormatterExtensions;
use crate::expression::parentheses::{empty_parenthesized, optional_parentheses, parenthesized};
use crate::{FormatNodeRule, PyFormatter};

#[derive(Default)]
pub struct FormatPatternMatchSequence;

#[derive(Debug)]
enum SequenceType {
Tuple,
TupleNoParens,
List,
}

impl FormatNodeRule<PatternMatchSequence> for FormatPatternMatchSequence {
fn fmt_fields(&self, item: &PatternMatchSequence, f: &mut PyFormatter) -> FormatResult<()> {
write!(
f,
[not_yet_implemented_custom_text(
"[NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]",
item
)]
)
let PatternMatchSequence { patterns, range } = item;
let sequence_type = match &f.context().source()[*range].chars().next() {
Some('(') => SequenceType::Tuple,
Some('[') => SequenceType::List,
_ => SequenceType::TupleNoParens,
};
let comments = f.context().comments().clone();
let dangling = comments.dangling(item);
if patterns.is_empty() {
return match sequence_type {
SequenceType::Tuple => empty_parenthesized("(", dangling, ")").fmt(f),
SequenceType::List => empty_parenthesized("[", dangling, "]").fmt(f),
SequenceType::TupleNoParens => {
unreachable!("If empty, it should be either tuple or list")
}
};
}
let items = format_with(|f| {
f.join_comma_separated(range.end())
.nodes(patterns.iter())
.finish()
});
match sequence_type {
SequenceType::Tuple => parenthesized("(", &items, ")")
.with_dangling_comments(dangling)
.fmt(f),
SequenceType::List => parenthesized("[", &items, "]")
.with_dangling_comments(dangling)
.fmt(f),
SequenceType::TupleNoParens => optional_parentheses(&items).fmt(f),
}
}
}
88 changes: 52 additions & 36 deletions ...on_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_complex.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ match x:
```diff
--- Black
+++ Ruff
@@ -2,105 +2,105 @@
@@ -2,97 +2,108 @@

# case black_test_patma_098
match x:
Expand Down Expand Up @@ -189,7 +189,7 @@ match x:
# case black_check_sequence_then_mapping
match x:
- case [*_]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
return "seq"
- case {}:
+ case {"NOT_YET_IMPLEMENTED_PatternMatchMapping": _, 2: _}:
Expand All @@ -202,8 +202,7 @@ match x:
- case {0: [1, 2, {}] | True} | {1: [[]]} | {0: [1, 2, {}]} | [] | "X" | {}:
+ case NOT_YET_IMPLEMENTED_PatternMatchOf | (y):
y = 1
- case []:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case []:
y = 2
# case black_test_patma_107
match x:
Expand Down Expand Up @@ -239,7 +238,7 @@ match x:
# case black_test_patma_185
match Seq():
- case [*_]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
y = 0
# case black_test_patma_063
match x:
Expand All @@ -257,23 +256,34 @@ match x:
# case black_test_patma_019
match (0, 1, 2):
- case [0, 1, *x, 2]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case [
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ *NOT_YET_IMPLEMENTED_PatternMatchStar,
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ ]:
y = 0
# case black_test_patma_052
match x:
- case [0]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case ["NOT_YET_IMPLEMENTED_PatternMatchValue"]:
y = 0
- case [1, 0] if (x := x[:0]):
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2] if (x := x[:0]):
+ case [
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ ] if (x := x[:0]):
y = 1
- case [1, 0]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case [
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ "NOT_YET_IMPLEMENTED_PatternMatchValue",
+ ]:
y = 2
# case black_test_patma_191
match w:
- case [x, y, *_]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
z = 0
# case black_test_patma_110
match x:
Expand All @@ -282,8 +292,7 @@ match x:
y = 0
# case black_test_patma_151
match (x,):
- case [y]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
@@ -100,7 +111,7 @@
z = 0
# case black_test_patma_114
match x:
Expand All @@ -292,7 +301,7 @@ match x:
y = 0
# case black_test_patma_232
match x:
@@ -108,7 +108,7 @@
@@ -108,7 +119,7 @@
y = 0
# case black_test_patma_058
match x:
Expand All @@ -301,27 +310,24 @@ match x:
y = 0
# case black_test_patma_233
match x:
@@ -116,11 +116,11 @@
y = 0
# case black_test_patma_078
@@ -118,9 +129,9 @@
match x:
- case []:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case []:
y = 0
- case [""]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case ["NOT_YET_IMPLEMENTED_PatternMatchValue"]:
y = 1
- case "":
+ case "NOT_YET_IMPLEMENTED_PatternMatchValue":
y = 2
# case black_test_patma_156
match x:
@@ -128,17 +128,17 @@
@@ -128,17 +139,17 @@
y = 0
# case black_test_patma_189
match w:
- case [x, y, *rest]:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
+ case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
z = 0
# case black_test_patma_042
match x:
Expand All @@ -336,8 +342,7 @@ match x:
- case {0: [1, 2, {}] | False} | {1: [[]]} | {0: [1, 2, {}]} | [] | "X" | {}:
+ case NOT_YET_IMPLEMENTED_PatternMatchOf | (y):
y = 1
- case []:
+ case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case []:
y = 2
```

Expand Down Expand Up @@ -370,7 +375,7 @@ match x:
y = 0
# case black_check_sequence_then_mapping
match x:
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
return "seq"
case {"NOT_YET_IMPLEMENTED_PatternMatchMapping": _, 2: _}:
return "map"
Expand All @@ -380,7 +385,7 @@ match x:
y = 0
case NOT_YET_IMPLEMENTED_PatternMatchOf | (y):
y = 1
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case []:
y = 2
# case black_test_patma_107
match x:
Expand Down Expand Up @@ -408,7 +413,7 @@ match x:
y = 2
# case black_test_patma_185
match Seq():
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
y = 0
# case black_test_patma_063
match x:
Expand All @@ -422,27 +427,38 @@ match x:
y = bar
# case black_test_patma_019
match (0, 1, 2):
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [
"NOT_YET_IMPLEMENTED_PatternMatchValue",
"NOT_YET_IMPLEMENTED_PatternMatchValue",
*NOT_YET_IMPLEMENTED_PatternMatchStar,
"NOT_YET_IMPLEMENTED_PatternMatchValue",
]:
y = 0
# case black_test_patma_052
match x:
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case ["NOT_YET_IMPLEMENTED_PatternMatchValue"]:
y = 0
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2] if (x := x[:0]):
case [
"NOT_YET_IMPLEMENTED_PatternMatchValue",
"NOT_YET_IMPLEMENTED_PatternMatchValue",
] if (x := x[:0]):
y = 1
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [
"NOT_YET_IMPLEMENTED_PatternMatchValue",
"NOT_YET_IMPLEMENTED_PatternMatchValue",
]:
y = 2
# case black_test_patma_191
match w:
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
z = 0
# case black_test_patma_110
match x:
case "NOT_YET_IMPLEMENTED_PatternMatchValue":
y = 0
# case black_test_patma_151
match (x,):
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [y]:
z = 0
# case black_test_patma_114
match x:
Expand All @@ -462,9 +478,9 @@ match x:
y = 0
# case black_test_patma_078
match x:
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case []:
y = 0
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case ["NOT_YET_IMPLEMENTED_PatternMatchValue"]:
y = 1
case "NOT_YET_IMPLEMENTED_PatternMatchValue":
y = 2
Expand All @@ -474,7 +490,7 @@ match x:
y = 0
# case black_test_patma_189
match w:
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
z = 0
# case black_test_patma_042
match x:
Expand All @@ -486,7 +502,7 @@ match x:
y = 0
case NOT_YET_IMPLEMENTED_PatternMatchOf | (y):
y = 1
case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
case []:
y = 2
```

Expand Down
Loading
Loading