Format PatternMatchSequence#6676
Conversation
| .nodes(patterns.iter()) | ||
| .finish() | ||
| }); | ||
| parenthesized("[", &items, "]") |
There was a problem hiding this comment.
Oh case (...) is also PatternMatchSequence. I need to fix this.
You can add additional tests to 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. |
PR Check ResultsBenchmarkLinuxWindows |
|
@MichaReiser Got it, I will add tests |
crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py
Show resolved
Hide resolved
harupy
force-pushed
the
format-FormatPatternSequence
branch
from
e9e5268 to
16fba95
Compare
crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py
Show resolved
Hide resolved
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
Outdated
Show resolved
Hide resolved
| } | ||
| SequenceType::TupleWithoutParentheses => { | ||
| let items = format_with(|f| { | ||
| f.join_with(&format_args![text(","), space()]) |
There was a problem hiding this comment.
Please add a comment why you're not using join_comma_separated here
crates/ruff_python_formatter/src/pattern/pattern_match_sequence.rs
Outdated
Show resolved
Hide resolved
| pass | ||
|
|
||
| match foo: | ||
| case "a", "b": |
There was a problem hiding this comment.
With join_comma_separated, this is formatted to:
case "NOT_YET_IMPLEMENTED_PatternMatchValue",
"NOT_YET_IMPLEMENTED_PatternMatchValue",:which is a syntax error.
There was a problem hiding this comment.
Should this be formatted like this?
case "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue":There was a problem hiding this comment.
or
case (
"NOT_YET_IMPLEMENTED_PatternMatchValue",
"NOT_YET_IMPLEMENTED_PatternMatchValue",
):
pass?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
i also think it would be good if we would break instead of writing an overlong lines.
+1
There was a problem hiding this comment.
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
There was a problem hiding this comment.
psf/black#3726 and psf/black#3403 seem related.
| }); | ||
| match sequence_type { | ||
| SequenceType::Tuple | SequenceType::TupleWithoutParentheses => { | ||
| parenthesized("(", &items, ")") |
There was a problem hiding this comment.
This is still not correct because parenthesized always wraps the sequence with ().
|
Did a quick read -- gonna merge to keep things moving along. Can always address follow-ups in new PRs. |
4 participants
Close #6645
Summary
Test Plan
Do I need to add extra tests?