WIP: Add Applicability to suggestion lints #2943
Conversation
| snippet(cx, haystack.span, ".."), | ||
| snippet(cx, needle.span, ".."))); | ||
| snippet(cx, needle.span, "..")), | ||
| Applicability::HasPlaceholders); |
There was a problem hiding this comment.
It only "maybe" has placeholders. We should make the snippet function return a tuple of Applicability and snippet-String
There was a problem hiding this comment.
The best thing to do here would be to have it accept &mut Applicability IMO
There was a problem hiding this comment.
Something like
let mut applicability = span.into(); // if macro-ish, makes it MaybeIncorrect
let foo = span_to_snippet(...., &mut applicability) // if Applicable, makes it HasPlaceholders in the macro case
|
FWIW http://github.com/kennytm/rustup-toolchain-install-master lets you locally build clippy on trunk |
Nice to know! Thanks! |
|
Thanks so much for doing this! |
|
needs a rebase |
|
Should I add the Applicability argument to the snippet function in this PR or in a separate one? |
phansch
added
the
S-waiting-on-review
flip1995
added
S-waiting-on-author
| "this `.fold` can be written more succinctly using another method", | ||
| "try", | ||
| sugg, | ||
| Applicability::HasPlaceholders, |
There was a problem hiding this comment.
Oh this is a stale PR to remind me (and others) that this should be done sometimes. When I'm (or someone else) continues working on this, I would start from scratch and go over these Applicability levels again (Also taking #3191 into consideration).
But yeah Applicability::MaybeIncorrect would probably be better here.
|
Closing in favor of #3459 |
6 participants
Resolves #2930
The applicability levels added to the
span_lint_and_suggandmultispan_sugg(do we need this onmultispan_sugg?) functions are how I felt would be the best fitting ones.I haven't tested this yet, because I wanted to wait for the new nightly instead of doing this manual. 😄
cc @Manishearth