Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
PR Check ResultsBenchmarkLinuxWindows |
dhruvmanila
force-pushed
the
dhruv/format-match-stmt
branch
from
e34d757 to
16fc898
Compare
dhruvmanila
force-pushed
the
dhruv/format-match-case
branch
from
65913e5 to
22e201d
Compare
dhruvmanila
force-pushed
the
dhruv/format-match-case
branch
from
ae9a0e7 to
51e0f67
Compare
| }; | ||
|
|
||
| // The new level is for the `case` nodes. | ||
| let mut f = WithNodeLevel::new(NodeLevel::CompoundStatement, f); |
There was a problem hiding this comment.
Otherwise, the node level is NodeLevel::TopLevel for the case nodes which is coming from the match statement. This will be problematic as the empty lines between case nodes should be at max 1.
| case NOT_YET_IMPLEMENTED_Pattern if foo == 2: # second | ||
| pass | ||
|
|
||
| case NOT_YET_IMPLEMENTED_Pattern if (foo := 1): # third |
There was a problem hiding this comment.
These parentheses are preserved in black as well.
There was a problem hiding this comment.
Oh actually, even if the line length is more than 88, this adds the parentheses thus not breaking on multiple lines. I'll have to look into it as using Parenthesize::IfRequired isn't correct because otherwise the parentheses in this example won't be preserved.
There was a problem hiding this comment.
This is solved. The heuristic is that if the expression already had parentheses, preserve that otherwise don't do anything even if it goes beyond the line length.
There was a problem hiding this comment.
You may want to use Parenthesize::Optional which preserves Parentheses. Whether you want to use guard.format() or maybe_parenthesize depends on if parentheses should always be added if they don't fit on the line or only if they don't bring their own parentheses (e.g. [a, b, c, d] doesn't need addtional parentheses because it has its own).
I would expect it to be maybe_parenthesize because it is used in all positions where expressions are formatted inside a statement.
There was a problem hiding this comment.
Parenthesize::Optional would break the expression into multiple lines which black doesn't do:
match long_lines:
case "this is a long line for if condition" if aaaaaaaaahhhhhhhh == 1 and bbbbbbaaaaaaaaaaa == 2: # comment
passRunning black on the above snippet wouldn't give any difference but if we use Optional, then we parenthesize the if expression:
match long_lines:
case NOT_YET_IMPLEMENTED_Pattern if (
aaaaaaaaahhhhhhhh == 1 and bbbbbbaaaaaaaaaaa == 2
): # comment
pass
crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py
Show resolved
Hide resolved
| if is_expression_parenthesized(guard.as_ref().into(), f.context().source()) { | ||
| guard.format().with_options(Parentheses::Always).fmt(f)?; | ||
| } else { | ||
| guard.format().fmt(f)?; | ||
| } |
There was a problem hiding this comment.
Preserving parentheses should be the default behavior if you call expression.format().
| if is_expression_parenthesized(guard.as_ref().into(), f.context().source()) { | |
| guard.format().with_options(Parentheses::Always).fmt(f)?; | |
| } else { | |
| guard.format().fmt(f)?; | |
| } | |
| guard.format().fmt(f)? |
Or you can use Parenthesize::Optional if it should use the same breaking logic as other expressions: e.g. that case a if a == [1, 2, 3] doesn't add parentheses and instead breaks after the [.
There was a problem hiding this comment.
Ok, thanks! Regarding your example, black have the same behavior where it breaks after [ instead of parenthesizing the expression.
| case NOT_YET_IMPLEMENTED_Pattern if foo == 2: # second | ||
| pass | ||
|
|
||
| case NOT_YET_IMPLEMENTED_Pattern if (foo := 1): # third |
There was a problem hiding this comment.
You may want to use Parenthesize::Optional which preserves Parentheses. Whether you want to use guard.format() or maybe_parenthesize depends on if parentheses should always be added if they don't fit on the line or only if they don't bring their own parentheses (e.g. [a, b, c, d] doesn't need addtional parentheses because it has its own).
I would expect it to be maybe_parenthesize because it is used in all positions where expressions are formatted inside a statement.
## Summary This PR adds formatting support for `MatchCase` node with subs for the `Pattern` nodes. ## Test Plan Added test cases for case node handling with comments, newlines. resolves: astral-sh#6299
3 participants
Summary
This PR adds formatting support for
MatchCasenode with subs for thePatternnodes.
Test Plan
Added test cases for case node handling with comments, newlines.
resolves: #6299