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.

Sign up for GitHub

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

Jump to bottom

Lists doc comments fix4041 #5560

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Lists doc comments fix4041 #5560

merged 5 commits into from
Feb 2, 2023

Conversation

thaqibm
Copy link
Contributor

@thaqibm thaqibm commented Oct 11, 2022 •
edited by ytmimi
Loading

Closes #4041

@thaqibm thaqibm mentioned this pull request Oct 11, 2022
Closed
ytmimi
ytmimi reviewed Oct 12, 2022
View reviewed changes
tests/config/issue-4041.toml Outdated
@@ -0,0 +1 @@
wrap_comments=true
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this file is necessary.

I would use a comment like //rustfmt-wrap_comments: true. The reason why wrap_comments=true isn't having any affect in the target file is because no line exceeds the default max width of 100 characters. I think we should add a tests/source/issue-4041.rs that is unformatted and has + lines that exceed 100 chars long.

@ytmimi
Copy link
Contributor

ytmimi commented Oct 12, 2022

Thanks for your first contribution to rustfmt 🎉

Take a look at the advice I gave about adding a test case that will utilize the wrap_comments=true config so that we can fully test that new behavior. Let me know if there are any questions, or you need help!

@ytmimi ytmimi self-assigned this Oct 12, 2022
@thaqibm
Copy link
Contributor Author

thaqibm commented Oct 13, 2022

Changed the test and added the source file.

@ytmimi
Copy link
Contributor

ytmimi commented Oct 13, 2022

The updated test looks exactly like we want!

I'm just now noticing that the doc comment on struct ItemizedBlock doesn't mention the + . When you have a moment could you update the doc comment above the struct.

@thaqibm
Copy link
Contributor Author

thaqibm commented Oct 14, 2022

updated the doc comment

ytmimi
ytmimi approved these changes Oct 14, 2022
View reviewed changes
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.

Thanks again for your help on this issue and for applying all the feedback 😁 If you're interested in taking on any other issues and need help don't hesitate to reach out!

I'm going to label this as ready to merge, but I'll hold off on actually merging as we're waiting for a few PRs to land in the rust-lang/rust repo.

@ytmimi ytmimi mentioned this pull request Oct 14, 2022
Merged
calebcartwright
calebcartwright approved these changes Feb 2, 2023
View reviewed changes
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 added release-notes Needs an associated changelog entry and removed pr-ready-to-merge labels Feb 2, 2023
@calebcartwright calebcartwright merged commit e2996a8 into rust-lang:master Feb 2, 2023
@calebcartwright calebcartwright removed the release-notes Needs an associated changelog entry label Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calebcartwright calebcartwright calebcartwright approved these changes

@ytmimi ytmimi ytmimi approved these changes

Assignees

@ytmimi ytmimi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rustfmt sometimes breaks lists in doc-comments
3 participants
@thaqibm @ytmimi @calebcartwright