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

Improve lint doc consistency #8954

Merged
merged 5 commits into from
Jun 9, 2022
Merged

Conversation

Serial-ATA
Copy link
Contributor

@Serial-ATA Serial-ATA commented Jun 5, 2022

changelog: none

This is a continuation of #8908.

Notable changes:

  • Removed empty Known Problems sections
  • Removed "Good"/"Bad" language (replaced with "Use instead")
  • Removed (and added some 😄) duplication
  • Ignored the [create_dir] example so it doesn't create clippy_lints/foo 😄

@rust-highfive
Copy link

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 5, 2022
@Serial-ATA Serial-ATA force-pushed the doc-comment-issues branch from 3a06ccf to 9aeed6b Compare June 5, 2022 20:03
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

I'm done with the first batch ~33 files or so. 🙃 Thank you for taking the time, this looks like you've actually read every description, which is impressive!

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Looks good overall, a few small comments. Everything should be simple to fix and then ready to be merged.

I hope I wasn't too nit-picky

Comment on lines +25 to +27
/// # let _: u64 =
/// 61864918973511
/// # ;
Copy link
Member

Choose a reason for hiding this comment

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

This is another weird let statements. I'll point them out, since I've understood you, that these should be reverted again, as they're not ideal. I'll copy and passta 🍝 this comment, so if you disagree, feel free to only state that once or on the specific case 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these ones, I thought it looked better to just show the literals themselves. Since it applies in all contexts I didn't think the let statement was all that important for the reader to see.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, I'm a bit torn, since I think it looks weird in the code, but the main focus should be the lint list documentation, where this will show correctly.

@xFrednet
Copy link
Member

xFrednet commented Jun 9, 2022

Alright, this version looks good to me. Thank you for the help and quick answers.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2022

📌 Commit 649ac36 has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Jun 9, 2022

⌛ Testing commit 649ac36 with merge 4970527...

@bors
Copy link
Contributor

bors commented Jun 9, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 4970527 to master...

@bors bors merged commit 4970527 into rust-lang:master Jun 9, 2022
@bors bors mentioned this pull request Jun 9, 2022
@Serial-ATA
Copy link
Contributor Author

@xFrednet thanks for getting the review done so quickly, really appreciate it! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants