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: reject leading ., ) without prefix as item marker #5839

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

xxchan
Copy link
Contributor

@xxchan xxchan commented Jul 17, 2023

fix #5835

@xxchan
Copy link
Contributor Author

xxchan commented Jul 29, 2023

cc @anforowicz to also take a look (who authorized #5423)

@xxchan
Copy link
Contributor Author

xxchan commented Aug 4, 2023

@ytmimi Could you take a look? Since the bug and fix are both very straightforward, and it's newly introduced. Thanks.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 5, 2023

@xxchan I'm traveling over the next few days, but I should have some time to review this early next week.

src/comment.rs Outdated
if prefix.len() <= 2 && prefix.chars().all(|c| char::is_ascii_digit(&c)) {
if (1..=2).contains(&prefix.len())
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. If the prefix.len() == 0 the condition would still pass. Good catch. I'm wondering if there's a more obvious way to write this since it took me a few seconds to figure out what (1..=2).contains(..) was doing.

Maybe assigning the condition to a meaningful variable name would help.

let has_leading_digits = (1..=2).contains(&prefix.len()) && prefix.chars().all(|c| char::is_ascii_digit(&c));

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

@xxchan thanks for making the edit!

To further prevent this regression it would be great to add the example from #5835 (comment) as a test case in tests/target/.

Happy to merge this after we add that test case and rebase on the latest master 😁

@calebcartwright
Copy link
Member

I haven't had a chance to look at this, but before merging, can we confirm this won't introduce a formatting diff (i.e. that this won't try to modify the output/result of a prior rustfmt run)?

@xxchan
Copy link
Contributor Author

xxchan commented Aug 25, 2023

can we confirm this won't introduce a formatting diff (i.e. that this won't try to modify the output/result of a prior rustfmt run)?

Yes, because it makes a condition triggering the fmt change more restrictive.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 25, 2023

@calebcartwright I ran the diff check job before the rebase and it failed ❌ for the same reason I explained in #5853 (comment).

I'm running the job again now that the branch has been rebased. I'm assuming we should be good now 🤞🏼, but I'll follow up once the job completes.

Edit:
diff check job ran successfully after the rebase ✅

@ytmimi
Copy link
Contributor

ytmimi commented Aug 25, 2023

@xxchan the issue mentions that wrap_comments=true needs to be set in order to trigger the bug. Can you update your test cases with // rustfmt-wrap_comments: true, and could you also add a test case using a doc comment.

@xxchan xxchan requested a review from ytmimi August 25, 2023 18:26
@ytmimi ytmimi merged commit f89cd3c into rust-lang:master Aug 29, 2023
@ytmimi
Copy link
Contributor

ytmimi commented Aug 29, 2023

@xxchan thanks again for your help on this one!

@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Aug 29, 2023
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Oct 23, 2023
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.

wrap_comments removes spaces after leading dot
4 participants