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.

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

Format PatternMatchOr #6905

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Conversation

LaBatata101
Copy link
Contributor

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

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

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
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.

@@ -167,45 +165,6 @@ pub fn format_node<'a>(
Ok(formatted)
}

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

Choose a reason for hiding this comment

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

Nice!

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

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
Copy link
Member

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

@MichaReiser MichaReiser enabled auto-merge (squash) August 28, 2023 08:00
@MichaReiser MichaReiser merged commit 99f4c68 into astral-sh:main Aug 28, 2023
16 checks passed
@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
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format PatternMatchOr
2 participants