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 PatternMatchStar #6653

Merged
merged 10 commits into from
Aug 24, 2023
Merged

Format PatternMatchStar #6653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
84ed8e8
Format PatternMatchStar
harupy Aug 23, 2023
4a54663
Add test
harupy Aug 23, 2023
38ab405
Address comments
harupy Aug 23, 2023
4554581
Handle dangling comments
harupy Aug 23, 2023
4d20b60
More tests
harupy Aug 23, 2023
1ec8d04
Handle comments between star and id
harupy Aug 23, 2023
940609b
Fix clippy error
harupy Aug 23, 2023
a554733
merge main
harupy Aug 23, 2023
2da6dea
snapshots
harupy Aug 23, 2023
c035020
Use dangling comments
charliermarsh Aug 24, 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
38 changes: 37 additions & 1 deletion 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 @@ -237,7 +237,6 @@ def foo():
]:
pass


match foo:
case 1:
y = 0
Expand All @@ -263,3 +262,40 @@ def foo():
# comment
):
y = 1

match foo:
case [1, 2, *rest]:
pass
case [1, 2, *_]:
pass
case [*rest, 1, 2]:
pass
case [*_, 1, 2]:
pass
case [
1,
2,
*rest,
]:
pass
harupy marked this conversation as resolved.
Show resolved Hide resolved
case [1, 2, * # comment
rest]:
pass
case [1, 2, * # comment
_]:
pass
Comment on lines +281 to +286
Copy link
Contributor Author

@harupy harupy Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is formatted first to:

    case [
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        *rest  # comment
        ,
    ]:
        pass
    case [
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        *_  # comment
        ,
    ]:
        pass

then formatted to:

    case [
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        *rest,  # comment
    ]:
        pass
    case [
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        *_,  # comment
    ]:
        pass

Copy link
Member

Choose a reason for hiding this comment

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

If you look at

match foo:
    case [* # comment
        rest, 1]:
        pass
    case [*rest # comment
          , 1]:
        pass

you get

{
    Node {
        kind: PatternMatchStar,
        range: 21..45,
        source: `* # comment⏎`,
    }: {
        "leading": [],
        "dangling": [
            SourceComment {
                text: "# comment",
                position: EndOfLine,
                formatted: true,
            },
        ],
        "trailing": [],
    },
    Node {
        kind: PatternMatchStar,
        range: 74..79,
        source: `*rest`,
    }: {
        "leading": [],
        "dangling": [],
        "trailing": [
            SourceComment {
                text: "# comment",
                position: EndOfLine,
                formatted: true,
            },
        ],
    },
}

so i think the best solution is to manually reassign the comment from dangling to trailing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@konstin Thanks for the comment. How can we access this leading-dangiling-trailing mapping?

Copy link
Member

Choose a reason for hiding this comment

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

@harupy - I typically run something like cargo run foo.py --emit=stdout --print-comments from ruff/crates/ruff_python_formatter.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry. You're asking how to modify these assignments. Any such modifications need to be done in placement.rs -- those are essentially custom rules that let us change the association of comments. See, e.g., handle_trailing_expression_starred_star_end_of_line_comment which is somewhat similar.

Copy link
Member

Choose a reason for hiding this comment

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

You have to add a case in handle_enclosed_comment for when the enclosing node is PatternMatchStar. The general logic to fix comment placement is in placement.rs/place_comment.

case [* # comment
rest, 1, 2]:
pass
case [* # comment
_, 1, 2]:
pass
case [* # end of line
# own line
_, 1, 2]:
pass
case [* # end of line
# own line
_, 1, 2]:
pass

15 changes: 15 additions & 0 deletions crates/ruff_python_formatter/src/comments/placement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ fn handle_enclosed_comment<'a>(
}
AnyNodeRef::WithItem(_) => handle_with_item_comment(comment, locator),
AnyNodeRef::PatternMatchAs(_) => handle_pattern_match_as_comment(comment, locator),
AnyNodeRef::PatternMatchStar(_) => handle_pattern_match_star_comment(comment),
AnyNodeRef::StmtFunctionDef(_) => handle_leading_function_with_decorators_comment(comment),
AnyNodeRef::StmtClassDef(class_def) => {
handle_leading_class_with_decorators_comment(comment, class_def)
Expand Down Expand Up @@ -1187,6 +1188,20 @@ fn handle_pattern_match_as_comment<'a>(
}
}

/// Handles dangling comments between the `*` token and identifier of a pattern match star:
///
/// ```python
/// case [
/// ...,
/// * # dangling end of line comment
/// # dangling end of line comment
/// rest,
/// ]: ...
/// ```
fn handle_pattern_match_star_comment(comment: DecoratedComment) -> CommentPlacement {
CommentPlacement::dangling(comment.enclosing_node(), comment)
}

/// Handles comments around the `:=` token in a named expression (walrus operator).
///
/// For example, here, `# 1` and `# 2` will be marked as dangling comments on the named expression,
Expand Down
33 changes: 24 additions & 9 deletions crates/ruff_python_formatter/src/pattern/pattern_match_star.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,34 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_formatter::{prelude::text, write, Buffer, FormatResult};
use ruff_python_ast::PatternMatchStar;

use crate::{not_yet_implemented_custom_text, FormatNodeRule, PyFormatter};
use crate::comments::{dangling_comments, SourceComment};
use crate::AsFormat;
use crate::{FormatNodeRule, PyFormatter};

#[derive(Default)]
pub struct FormatPatternMatchStar;

impl FormatNodeRule<PatternMatchStar> for FormatPatternMatchStar {
fn fmt_fields(&self, item: &PatternMatchStar, f: &mut PyFormatter) -> FormatResult<()> {
write!(
f,
[not_yet_implemented_custom_text(
"*NOT_YET_IMPLEMENTED_PatternMatchStar",
item
)]
)
let PatternMatchStar { name, .. } = item;

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

write!(f, [text("*"), dangling_comments(dangling)])?;

match name {
Some(name) => write!(f, [name.format()]),
None => write!(f, [text("_")]),
}
}

fn fmt_dangling_comments(
&self,
_dangling_comments: &[SourceComment],
_f: &mut PyFormatter,
) -> FormatResult<()> {
// Handled by `fmt_fields`
Ok(())
}
}
43 changes: 9 additions & 34 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 @@ -179,8 +179,7 @@ match x:
y = 0
# case black_check_sequence_then_mapping
match x:
- case [*_]:
+ case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [*_]:
return "seq"
- case {}:
+ case {"NOT_YET_IMPLEMENTED_PatternMatchMapping": _, 2: _}:
Expand All @@ -204,7 +203,7 @@ match x:
x = True
# case black_test_patma_154
match x:
@@ -54,15 +54,15 @@
@@ -54,11 +54,11 @@
y = 0
# case black_test_patma_134
match x:
Expand All @@ -219,12 +218,7 @@ match x:
y = 2
# case black_test_patma_185
match Seq():
- case [*_]:
+ case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
y = 0
# case black_test_patma_063
match x:
@@ -72,11 +72,11 @@
@@ -72,7 +72,7 @@
y = 1
# case black_test_patma_248
match x:
Expand All @@ -233,26 +227,7 @@ match x:
y = bar
# case black_test_patma_019
match (0, 1, 2):
- case [0, 1, *x, 2]:
+ case [0, 1, *NOT_YET_IMPLEMENTED_PatternMatchStar, 2]:
y = 0
# case black_test_patma_052
match x:
@@ -88,7 +88,7 @@
y = 2
# case black_test_patma_191
match w:
- case [x, y, *_]:
+ case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
z = 0
# case black_test_patma_110
match x:
@@ -128,17 +128,17 @@
y = 0
# case black_test_patma_189
match w:
- case [x, y, *rest]:
+ case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
@@ -132,13 +132,13 @@
z = 0
# case black_test_patma_042
match x:
Expand Down Expand Up @@ -300,7 +275,7 @@ match x:
y = 0
# case black_check_sequence_then_mapping
match x:
case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [*_]:
return "seq"
case {"NOT_YET_IMPLEMENTED_PatternMatchMapping": _, 2: _}:
return "map"
Expand Down Expand Up @@ -338,7 +313,7 @@ match x:
y = 2
# case black_test_patma_185
match Seq():
case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [*_]:
y = 0
# case black_test_patma_063
match x:
Expand All @@ -352,7 +327,7 @@ match x:
y = bar
# case black_test_patma_019
match (0, 1, 2):
case [0, 1, *NOT_YET_IMPLEMENTED_PatternMatchStar, 2]:
case [0, 1, *x, 2]:
y = 0
# case black_test_patma_052
match x:
Expand All @@ -364,7 +339,7 @@ match x:
y = 2
# case black_test_patma_191
match w:
case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [x, y, *_]:
z = 0
# case black_test_patma_110
match x:
Expand Down Expand Up @@ -404,7 +379,7 @@ match x:
y = 0
# case black_test_patma_189
match w:
case [x, y, *NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [x, y, *rest]:
z = 0
# case black_test_patma_042
match x:
Expand Down
36 changes: 9 additions & 27 deletions ...hon_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ match bar1:
...
case another:
...
@@ -32,23 +32,32 @@
@@ -32,14 +32,23 @@
match maybe, multiple:
case perhaps, 5:
pass
Expand All @@ -188,11 +188,9 @@ match bar1:
pass
case _:
pass


@@ -48,7 +57,7 @@
match a, *b, c:
- case [*_]:
+ case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [*_]:
assert "seq" == _
- case {}:
+ case {"NOT_YET_IMPLEMENTED_PatternMatchMapping": _, 2: _}:
Expand All @@ -213,13 +211,7 @@ match bar1:
pass

case [a as match]:
@@ -80,17 +84,14 @@


match a, *b(), c:
- case d, *f, g:
+ case d, *NOT_YET_IMPLEMENTED_PatternMatchStar, g:
pass
@@ -85,12 +89,9 @@


match something:
Expand All @@ -234,17 +226,12 @@ match bar1:
pass


@@ -101,19 +102,22 @@
@@ -101,19 +102,17 @@
case 2 as b, 3 as c:
pass

- case 4 as d, (5 as e), (6 | 7 as g), *h:
+ case (
+ 4 as d,
+ 5 as e,
+ NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g,
+ *NOT_YET_IMPLEMENTED_PatternMatchStar,
+ ):
+ case 4 as d, 5 as e, NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g, *h:
pass


Expand Down Expand Up @@ -324,7 +311,7 @@ match (


match a, *b, c:
case [*NOT_YET_IMPLEMENTED_PatternMatchStar]:
case [*_]:
assert "seq" == _
case {"NOT_YET_IMPLEMENTED_PatternMatchMapping": _, 2: _}:
assert "map" == b
Expand Down Expand Up @@ -353,7 +340,7 @@ match match:


match a, *b(), c:
case d, *NOT_YET_IMPLEMENTED_PatternMatchStar, g:
case d, *f, g:
pass


Expand All @@ -371,12 +358,7 @@ match something:
case 2 as b, 3 as c:
pass

case (
4 as d,
5 as e,
NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g,
*NOT_YET_IMPLEMENTED_PatternMatchStar,
):
case 4 as d, 5 as e, NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g, *h:
pass


Expand Down
22 changes: 3 additions & 19 deletions ...hon_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_simple.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,23 +104,7 @@ def where_is(point):
```diff
--- Black
+++ Ruff
@@ -23,7 +23,7 @@
# The rest of your commands go here

match command.split():
- case ["drop", *objects]:
+ case ["drop", *NOT_YET_IMPLEMENTED_PatternMatchStar]:
for obj in objects:
character.drop(obj, current_room)
# The rest of your commands go here
@@ -33,24 +33,24 @@
pass
case ["go", direction]:
print("Going:", direction)
- case ["drop", *objects]:
+ case ["drop", *NOT_YET_IMPLEMENTED_PatternMatchStar]:
print("Dropping: ", *objects)
case _:
@@ -39,18 +39,18 @@
print(f"Sorry, I couldn't understand {command!r}")

match command.split():
Expand Down Expand Up @@ -217,7 +201,7 @@ match command.split():
# The rest of your commands go here

match command.split():
case ["drop", *NOT_YET_IMPLEMENTED_PatternMatchStar]:
case ["drop", *objects]:
for obj in objects:
character.drop(obj, current_room)
# The rest of your commands go here
Expand All @@ -227,7 +211,7 @@ match command.split():
pass
case ["go", direction]:
print("Going:", direction)
case ["drop", *NOT_YET_IMPLEMENTED_PatternMatchStar]:
case ["drop", *objects]:
print("Dropping: ", *objects)
case _:
print(f"Sorry, I couldn't understand {command!r}")
Expand Down
Loading
Loading