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 Issue 'Trailing comma is duplicated when obstructed by comments' #4520

Merged
merged 7 commits into from
Feb 27, 2021

Conversation

whizsid
Copy link
Contributor

@whizsid whizsid commented Nov 10, 2020

Fixes #3876 .

Comment on lines 1 to 14
fn f<S>(s: S)
where
S: Send /* */,
//
{
}

fn main() {
foo(
x // comment 1
,
// comment 2
)
}
Copy link
Member

@calebcartwright calebcartwright Nov 11, 2020

Choose a reason for hiding this comment

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

This is a slight improvement in that the commas are not duplicated, though the formatting doesn't feel quite right. In particular I'm confused by the indentation/alignment in the first snippet, as well as the position of the , in the second.

I think a big question is whether there's a valid use case for comments before the comma, or whether that's just unintentional i.e.:

fn main() {
    foo(
        x, // comment 1
        // comment 2
    )
}

// or
fn main() {
    foo(
        x,
        // comment 1
        // comment 2
    )
}

although at the moment I suspect rustfmt may want to do this, though that's largely due to a bug in default visual alignment for comments (refs #4108)

fn main() {
    foo(
        x, // comment 1
           // comment 2
    )
}

@whizsid
Copy link
Contributor Author

whizsid commented Feb 19, 2021

Sorry I was away from open source in last months. @calebcartwright Can you review again?

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks for this! It's in pretty good shape though think we can optimize it a bit and have also requested some additional tests

src/formatting/lists.rs Outdated Show resolved Hide resolved
src/formatting/lists.rs Outdated Show resolved Hide resolved
tests/source/issue-3876.rs Show resolved Hide resolved
@calebcartwright
Copy link
Member

Excellent thank you!

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.

Trailing comma is duplicated when obstructed by comments
3 participants