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

make a bunch of lints texts adhere to rustc dev guide #5888

Merged
merged 27 commits into from
Aug 10, 2020

Conversation

matthiaskrgr
Copy link
Member

According to the rustc-dev guide: "The text should be matter of fact and avoid capitalization and periods, unless multiple sentences are needed"

changelog: make some lint output adhere to the rustc-dev guide

@rust-highfive
Copy link

r? @yaahc

(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 Aug 10, 2020
Copy link
Member

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

🥳

@@ -79,7 +79,7 @@ impl<'tcx> LateLintPass<'tcx> for NoNegCompOpForPartialOrd {
cx,
NEG_CMP_OP_ON_PARTIAL_ORD,
expr.span,
"The use of negated comparison operators on partially ordered \
"the use of negated comparison operators on partially ordered \
Copy link
Member

Choose a reason for hiding this comment

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

this one still has sentences and punctuation but had the initial capitalization removed. I'm not sure which direction to go on this one but we should be consistent.

Copy link
Member Author

@matthiaskrgr matthiaskrgr Aug 10, 2020

Choose a reason for hiding this comment

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

I was also not sure... The guide says The text should be matter of fact and avoid capitalization and periods, unless multiple sentences are needed which is why I left it mostly untouched.

Copy link
Member

@yaahc yaahc Aug 10, 2020

Choose a reason for hiding this comment

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

Makes sense, it looks like you already applied the change to turn the multiple sentences into 1 longer non sentence. I think it might also be okay to revert it back to the original punctuation / capitalization version. I'll leave it up to you to pick which one you prefer.

Once you've made the decision feel free to r+ for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think converting all multi-sentence warnings into uber-long single-sentence warnings would probably hurt readability too much...

clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
Copy link
Member

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

actually, caught a few

@matthiaskrgr
Copy link
Member Author

@bors r=yaahc

@bors
Copy link
Contributor

bors commented Aug 10, 2020

📌 Commit 6d0b5e2 has been approved by yaahc

@bors
Copy link
Contributor

bors commented Aug 10, 2020

⌛ Testing commit 6d0b5e2 with merge cc5bfd4...

@bors
Copy link
Contributor

bors commented Aug 10, 2020

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

@bors bors merged commit cc5bfd4 into rust-lang:master Aug 10, 2020
bors added a commit that referenced this pull request Aug 13, 2020
…atthiaskrgr

Add reference to rustc-dev-guide about lint message

I think it would be better to add lint message convention to documentation. I referred to #5888 and #5893.

changelog: none
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