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

Determine when new comment lines are needed for itemized blocks #5097

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Nov 20, 2021

Fixes #5088

Previously, rustfmt would add a new comment line anytime it reformatted
an itemized block within a comment. This would lead to rustfmt adding
empty comments with trailing whitespace.

Now, new comment lines are only added if the original comment spanned
multiple lines, if the comment needs to be wrapped, or if the comment
originally started with an empty comment line.

@calebcartwright
Copy link
Member

Nice! I think this is probably the right route to go, though out of an abundance of caution, could you add some additional test cases (above and beyond what you already have) which will actually hit the width boundaries and force a wrap? That should be doable either via some longer inputs (and/or with added indentation levels/nested items/etc.), or by setting comment_width to a sufficiently low value

@ytmimi
Copy link
Contributor Author

ytmimi commented Nov 23, 2021

could you add some additional test cases which will actually hit the width boundaries and force a wrap? That should be doable either via some longer inputs (and/or with added indentation levels/nested items/etc.), or by setting comment_width to a sufficiently low value

Sure thing. I'll include some more tests for this PR!

@ytmimi ytmimi force-pushed the issue_5088 branch 2 times, most recently from 33260a1 to b6d6a2e Compare November 24, 2021 22:51
@ytmimi
Copy link
Contributor Author

ytmimi commented Nov 24, 2021

@calebcartwright I've gone ahead and added the additional test cases that force the comments to wrap. let me know if there's anything else you feel we should be testing to round out the PR.

@calebcartwright
Copy link
Member

One case where the proposed changes would be problematic would be when an itemized block opens with an empty comment line, e.g.

(note no trailing spaces after the first opener)

//
// - some itemized block 1
// - some itemized block 2

Haven't dug into the cause, but that input with the proposed changes would result in the first line getting stripped, and the opening space in the first item getting dropped, e.g.

//- some itemized block 1
// - some itemized block 2

So we'll need to iterate on this a bit more

@ytmimi
Copy link
Contributor Author

ytmimi commented Nov 29, 2021

I went back and took a look at this. There was a logical bug in buffer_contains_comment. Basically we weren't taking into account the scenario that you posed where the comment buffer already contained an empty comment. I fixed that issue and added some clarifying comments to that method. Also included your input in two new test cases!

@calebcartwright
Copy link
Member

Looks good, thanks for following up. My last ask (perhaps more of a question than a request) is whether it'd be worthwhile to include any of these tests but in the default mode (wrap_comments disabled)? I wouldn't anticipate any issues currently, but could see it potentially being useful in the future as a defensive type of test should this part of the codebase be modified

@ytmimi
Copy link
Contributor Author

ytmimi commented Nov 30, 2021

Added warp_comments=false version of all the tests. I agree that these could be helpful in the future to make sure that we maintain the same behavior

Fixes 5088

Previously, rustfmt would add a new comment line anytime it reformatted
an itemized block within a comment when ``wrap_comments=true``. This
would lead to rustfmt adding empty comments with trailing whitespace.

Now, new comment lines are only added if the original comment spanned
multiple lines, if the comment needs to be wrapped, or if the comment
originally started with an empty comment line.
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!

@calebcartwright calebcartwright merged commit ec46ffd into rust-lang:master Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Added comment line above any comment that starts with -
2 participants