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

Remove extra whitespace after match arrow #5072

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mujpao
Copy link
Contributor

@mujpao mujpao commented Nov 9, 2021

Fixes #4844

When using control_brace_style = 'AlwaysNextLine' and match_arm_blocks = false, rustfmt would add extra whitespace if the match body didn't fit on one line.

This adds a check for this case.

When using `control_brace_style = 'AlwaysNextLine'` and
`match_arm_blocks = false`, rustfmt would add extra whitespace if the
match body didn't fit on one line.

This adds a check for this case.
@ytmimi
Copy link
Contributor

ytmimi commented Nov 9, 2021

Thanks for the PR!

From my initial look at the proposed changes, things look good to me! From the linked issue, I understand that this bug only occurs when control_brace_style = 'AlwaysNextLine' is used with match_arm_blocks = false. I do wonder if it's worthwhile to test different combinations of control_brace_style and match_arm_blocks, but I realize that's probably outside the scope of the issue you're solving here.

I do want to call out that there seems to be an open issue (#4896) and PR (#4924) that would move to deprecate match_arm_blocks, but there hasn't been progress on the PR since Sep 08, 2021.

@mujpao
Copy link
Contributor Author

mujpao commented Nov 9, 2021

I could add some more tests. Even if match_arm_blocks gets deprecated, the tests could still be relevant, right?

I was just looking through the issues, and I realized that #4844 is probably a duplicate of #4003.

Also, I noticed that rustfmt will preserve a newline between match arms, like this (without any options):

fn main() {
    let fooooooo = "100000000000000000000000";
    let _bar = match fooooooo {
        "100000000000000000000000" => {
            fooooooo.len() == 1 && fooooooo.contains("222222222222222222")
        }

        _ => unreachable!("Should not happen"),
    };
}

Is this what's supposed to happen?

@ytmimi
Copy link
Contributor

ytmimi commented Nov 11, 2021

I could add some more tests. Even if match_arm_blocks gets deprecated, the tests could still be relevant, right?

I'm personally a fan of having more tests, so I'd encourage you to add more, but as I mentioned, I wouldn't want you to have to test things that aren't related to the issue.

I was just looking through the issues, and I realized that 4844 is probably a duplicate of 4003.

Good catch on noticing the duplicate issue!

Also, I noticed that rustfmt will preserve a newline between match arms, like this (without any options):
Is this what's supposed to happen?

That's a really good question. I'm not as familiar with the match arm formatting, but I'd assume that there shouldn't be a newline between match arms. @calebcartwright, do you have a better idea on whether this is the intended behavior or not?

@mujpao
Copy link
Contributor Author

mujpao commented Nov 12, 2021

When adding more tests, should I force push or add another commit?

@calebcartwright calebcartwright self-assigned this Nov 13, 2021
@calebcartwright
Copy link
Member

Is this what's supposed to happen?
I'd assume that there shouldn't be a newline between match arms. @calebcartwright, do you have a better idea on whether this is the intended behavior or not?

tbh I wouldn't have expected it to preserve a newline between arms in cases where there was one or more there before. however, the relevant section in the Style Guide doesn't provide any explicit guidance so this is definitely a case where we want to stick with the status quo to avoid introducing any "breaking" formatting changes

@calebcartwright
Copy link
Member

When adding more tests, should I force push or add another commit?

Great question, thanks for asking! In this specific case, it's up to you.

In general, I strongly dislike merge commits so outside the subtree syncs (which require merge commits), all PRs are merged with a rebase or squash strategy.With a rebase strategy, that requires the commits be well structured, an admittedly vague/abstract definition 😄 It's fine for a PR to have multiple commits provided they are logically structured (e.g. not a 7 commit PR for a 5 line diff), and that structure can always be discussed in the context of a specific PR. And I can always do a squash merge in cases where I feel like the PR commits can be condensed into a single one.

@ytmimi ytmimi added the p-low label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

control_brace_style and match_arm_blocks incompatability
3 participants