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

Fixed trailing comment being moved when comma is inserted #4655

Open
wants to merge 3 commits into
base: rustfmt-2.0.0-rc.2
Choose a base branch
from

Conversation

vallentin
Copy link
Contributor

@vallentin vallentin commented Jan 18, 2021

Fixes #4654

The trailing comma being inserted no longer causes a subsequent comment to be moved up.

This change does not affect the trailing_comma configuration.


This change broke issue-3532.rs (#3532). I have modified the test case, as I thought that was appropriate.

In short, the test asserted that the comment was moved up. However, with multiple match arms, that is inconsistent formatted, as it would only do so for the last match arm. This change also fixes that, and thus I modified the test case as well.

Unformatted Before Now
unformatted before now

Note if comment_end == ")" && !leave_last { was added, to not break foo_one_post() in issue-3198.rs (#3198) as otherwise it would add a newline along with a trailing comma.

@vallentin vallentin changed the title Fixes trailing comment being moved when comma is inserted Fixed trailing comment being moved when comma is inserted Jan 19, 2021
@calebcartwright
Copy link
Member

Thanks for the report and opening a corresponding PR!

This change broke issue-3532.rs (#3532). I have modified the test case, as I thought that was appropriate.

Will hopefully have some time over the next couple days to review the changes in more detail, though this is one minor thing we'll want to change. Could you move your enhanced version of this test case into a new separate file and leave the original source file as-is? (understand the target/output formatting for that source would be adjusted by these changes)

It'll be great to extend the test cases, but we like to keep the issue-mapped source/input files identical to what was provided in the respective issue as a general practice.

@vallentin
Copy link
Contributor Author

vallentin commented Jan 22, 2021

@calebcartwright Thanks for the response. I'm in no hurry, so review whenever you or anybody else has time! 😄

leave the original source file

I'll move the extended version into a new file. Any ideas for a name?

Just one thing I need clarified. Do I still modify target/issue-3532.rs? As otherwise that test case would break.

fn foo(a: T) {
match a {
1 => {}
0 => {} // _ => panic!("doesn't format!"),
}
}

Because given source/issue-3532.rs as is:

fn foo(a: T) {
match a {
1 => {}
0 => {}
// _ => panic!("doesn't format!"),
}
}

Then this PR changes, such that it formats into:

fn foo(a: T) {
    match a {
        1 => {}
        0 => {}
        // _ => panic!("doesn't format!"),
    }
}

@calebcartwright
Copy link
Member

Just one thing I need clarified. Do I still modify target/issue-3532.rs? As otherwise that test case would break.

Yup, that's perfectly alright, just want to keep the source matching the issue report. Generally speaking we wouldn't expect to see the target formatting for older issues updated either, though there's certainly exceptions for that in cases where there may be a separate bug and/or subsequent formatting improvements as seems to be the case here.

I'll move the extended version into a new file. Any ideas for a name?

That doesn't matter too much. Would be alright to stick it in the new file for this latest 4654 issue, or even just plug it in the match catch-all file in https://github.com/rust-lang/rustfmt/blob/master/tests/source/match.rs

@vallentin
Copy link
Contributor Author

vallentin commented Jan 22, 2021

I have rebased and restored source/issue-3532.rs and only fixed the affected part of target/issue-3532.rs.

I added the "extended match version" to the new test case I added.

match a {
0 => {}
// Foo
1 => {} // Bar
// Baz
2 => {}
// Qux
}

@calebcartwright
Copy link
Member

Note if comment_end == ")" && !leave_last { was added, to not break foo_one_post() in issue-3198.rs (#3198) as otherwise it would add a newline along with a trailing comma.

Thanks for including this note. I did a quick pass through the changes and was rather confused by this one. Something about this feels off and I'm not convinced just yet we'd want to proceed with this odd side effect handling. Perhaps some of the changes made originally to deal with #3198 need to be adjusted to account for the changes here?

@calebcartwright calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:23
@ytmimi ytmimi added pr-targeting-2.0 This PR is targeting the 2.0 branch pr-merge-conflict This PR has merge conflicts that must be resolved p-low labels Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-low pr-merge-conflict This PR has merge conflicts that must be resolved pr-targeting-2.0 This PR is targeting the 2.0 branch pr-waiting-on-author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing trailing comma in struct causes subsequent comment to be moved
3 participants