Fix #5533: Prefer light_rewrite_comment if it is not a doccomment

[pan93412]

This pull request adapts the plan 2 of @ytmimi's comment, and it won't touch any comment which is not a documentation comment now.

However, it did not fixed the root cause (which is partially indicated in Plan 1): block_comment is true if the length is too long…?. As it seems not related to #5533, it may be fixed in another PR.

[pan93412]

cc @ytmimi . Thanks!

[ytmimi]

Thanks for your first contribution to rustfmt 🎉

See my inline comments for thoughts on how we can simplify the PR. Let me know if you have any questions.

[ytmimi]

Thanks for the follow up!

I still believe is_doc_comment is the only boolean we need to include in our check to force code execution to move to light_rewrite_comment. I could be missing something, and if we need to check both is_doc_comment and style.is_doc_comment could you please explain why. The comment is helpful and I liked that you kept it 👍🏼

[pan93412]

Thanks for the follow up!

I still believe is_doc_comment is the only boolean we need to include in our check to force code execution to move to light_rewrite_comment. I could be missing something, and if we need to check both is_doc_comment and style.is_doc_comment could you please explain why. The comment is helpful and I liked that you kept it 👍🏼

I have reverted it. However, why did L384 do such a double check?

rustfmt/src/comment.rs

Lines 378 to 385 in 8919fd3

	rewrite_comment_inner(
	first_group,
	block_style,
	style,
	shape,
	config,
	is_doc_comment || style.is_doc_comment(),
	)?

Seems like this code had been refactored before, and I don't know why they wrote it. Besides, even when I removed the determination, all the tests were still passed.

[ytmimi]

why did L384 do such a double check?

That decision predates my time with the project. My best guess is that there was a time when both were necessary to be absolutely sure we were rewriting a doc comment. Now that we have rewrite_comment and rewrite_doc_comment as our top level comment rewriting functions it's easier to differentiate what kind of comment we're rewriting.

Seems like this code had been refactored before, and I don't know why they wrote it. Besides, even when I removed the determination, all the tests were still passed.

It's might be the case that style.is_doc_comment() is no longer needed, but it's not hurting us right now, so we'll keep it for the time being.

[ytmimi]

Thank you for your help on this! I think we're good to go here but will hold off on merging to give @calebcartwright a chance to take a look.

[ytmimi]

@pan93412 I noticed that this PR also resolves #5568 so I added a test case on your branch to validate that

[pan93412]

@pan93412 I noticed that this PR also resolves #5568 so I added a test case on your branch to validate that

Thanks!

[lopopolo]

Hey folks, I think this PR has the ready to merge label and it resolves several issues with formatting comments. What else needs to be done to get this merged?

[ytmimi]

@pan93412 thanks again for your help on this one. This fixes a few issues so I'm going to merge.

Diff check job ran successfully ✅

[mightyiam]

Thank you.