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

fix: remove multiple trailing lines in comments #6163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

overhacked
Copy link

Previously, rustfmt would remove one blank, trailing line in a comment every time it ran, so formatting was not idempotent if a comment had multiple trailing lines. Comments are only reformatted in this way if comment::rewrite_comment_inner is called, which is enabled by any of the config options normalize_comments, wrap_comments, or format_code_in_doc_comments (for doc comments only). Absent those config options, no trailing line reformatting occurs at all.

In this fix, when the existing condition that detects a blank, trailing line is true, any preceding blank lines are removed from the reformatted comment. A source/target "system test" was added to prevent regression.

Previously, rustfmt would remove one blank, trailing line in a comment
every time it ran, so formatting was not idempotent if a comment had
multiple trailing lines. Comments are only reformatted in this way if
`comment::rewrite_comment_inner` is called, which is enabled by any of the
config options `normalize_comments`, `wrap_comments`, or
`format_code_in_doc_comments` (for doc comments only). Absent those
config options, no trailing line reformatting occurs at all.

In this commit, when the existing condition that detects a blank, trailing
line is true, any preceding blank lines are removed from the reformatted
comment. A source/target "system test" was added to prevent regression.

Signed-off-by: Ross Williams <ross@ross-williams.net>
@ytmimi
Copy link
Contributor

ytmimi commented May 21, 2024

@overhacked thanks for the PR. Would you mind also filing an issue for this too.

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.

None yet

3 participants