-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add Applicability to suggestion lints: Take 2 #3459
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
Conversation
Some bugs and some documentation is unrelated to the Applicability change, but these bugs were serious and the documentation was kind of required to understand what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit.
Lets do this!
@@ -300,6 +301,7 @@ impl WarningType { | |||
"mistyped literal suffix", | |||
"did you mean to write", | |||
grouping_hint.to_string(), | |||
Applicability::MachineApplicable, |
There was a problem hiding this comment.
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 should be machine applicable. It's just a "looks wrong, needs user attention, probably should be X"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So MaybeIncorrect
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small nit.
clippy_lints/src/bytecount.rs
Outdated
format!("bytecount::count({}, {})", | ||
snippet_with_applicability(cx, haystack.span, "..", &mut applicability), | ||
snippet_with_applicability(cx, needle.span, "..", &mut applicability)), | ||
Applicability::Unspecified, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that read applicability
here?
This is #2943 completely redone, so that we finally have applicability levels for our suggestion lints and #2943 can finally be closed.
Resolves #2930
Implements #2943 (comment)
Still open:
multispan_sugg
(Should this also be done in this PR?)PS: Sorry for some unrelated formatting changes, those just slipped in ¯\_(ツ)_/¯