Skip to content

Comments

Format PatternMatchOr#6905

Merged
MichaReiser merged 2 commits intoastral-sh:mainfrom
LaBatata101:format-pattern-match-or
Aug 28, 2023
Merged

Format PatternMatchOr#6905
MichaReiser merged 2 commits intoastral-sh:mainfrom
LaBatata101:format-pattern-match-or

Conversation

@LaBatata101
Copy link
Contributor

@LaBatata101 LaBatata101 commented Aug 26, 2023

Summary

There was only one case where I couldn't figure it out how to format properly, which is the following

match x:
    case (
        (a)
        | # trailing
        ( b )
    ):
        ...

That code is being formatted to this

match x:
    case (
        (
            a  # trailing
        )
        | (b)
    ):
        ...

But it should be formatted to this

match x:
    case (
        (a)  # trailing
        | (b)
    ):
        ...

Closes #6643

Test Plan

cargo test

@LaBatata101
Copy link
Contributor Author

LaBatata101 commented Aug 26, 2023 •
edited
Loading

Now clippy complains about not_yet_implemented_custom_text never being used. Should this function be removed?

And look at that amount of lines removed ☺️

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 26, 2023

CodSpeed Performance Report

Merging #6905 will not alter performance

Comparing LaBatata101:format-pattern-match-or (25e5224) with main (eae59cf)

Summary

✅ 16 untouched benchmarks

@MichaReiser
Copy link
Member

MichaReiser commented Aug 27, 2023

Now clippy complains about not_yet_implemented_custom_text never being used. Should this function be removed?

And look at that amount of lines removed ☺️

Finally, our last syntax is now implemented. Yes, we can remove that function.

@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 27, 2023
MichaReiser
MichaReiser approved these changes Aug 28, 2023
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Neat! Thanks for working on the last pattern.

crates/ruff_python_formatter/src/lib.rs
Ok(formatted)
}

pub(crate) struct NotYetImplementedCustomText<'a> {
Copy link
Member

@MichaReiser MichaReiser Aug 28, 2023

Choose a reason for hiding this comment

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

Nice!

crates/ruff_python_formatter/src/pattern/pattern_match_or.rs
let leading_value_comments = comments.leading(pattern);
// Format the expressions leading comments **before** the operator
if leading_value_comments.is_empty() {
write!(f, [in_parentheses_only_soft_line_break_or_space()])?;
Copy link
Member

@MichaReiser MichaReiser Aug 28, 2023

Choose a reason for hiding this comment

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

Using the in_parentheses_only builder here seems correct but isn't sufficient without using optional_parentheses in the match case formatting. I created an issue to track that work #6933

@MichaReiser MichaReiser force-pushed the format-pattern-match-or branch from 53efe4d to 9b22f0b Compare August 28, 2023 07:57
@MichaReiser
Copy link
Member

MichaReiser commented Aug 28, 2023

This PR improves the similarity index for CPython from cpython 0.76076 to 0.76081

@MichaReiser MichaReiser force-pushed the format-pattern-match-or branch from 9b22f0b to f5f7ddf Compare August 28, 2023 08:00
@MichaReiser MichaReiser enabled auto-merge (squash) August 28, 2023 08:00
@MichaReiser MichaReiser merged commit 99f4c68 into astral-sh:main Aug 28, 2023
@cnpryer cnpryer mentioned this pull request Aug 28, 2023
Closed
@LaBatata101 LaBatata101 deleted the format-pattern-match-or branch December 1, 2023 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Format PatternMatchOr

2 participants

@LaBatata101 @MichaReiser