Implement new lint: if_then_some_else_none #6859
Conversation
|
r? @Manishearth (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
I think we should use msrv for this lint because the suggested bool::then was introduced in 1.50.0.
https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md#specifying-the-lints-minimum-supported-rust-version-msrv
|
@giraffate thanks for pointing that out, that's absolutely correct! Fixed it now. |
|
r? @giraffate do you want to take a shot at reviewing this? |
|
Yes, I'll do! |
|
@bors delegate=giraffate |
|
✌️ @giraffate can now approve this pull request |
| let cond_snip = utils::snippet(cx, cond.span, "[condition]"); | ||
| let arg_snip = utils::snippet(cx, then_arg.span, ""); |
There was a problem hiding this comment.
Can test codes like below be added? Using macro, the suggested code may be broken, so it would be better to use the method like snippet_with_macro_callsite.
let _ = if matches!(true, true) {
println!("true!");
Some(matches!(true, false))
} else {
None
};Additionally, can test codes like below be added? The parenthesis isn't considered, so the suggested codes seem not to work.
let x = Some(5);
let _ = x.and_then(|o| if o < 32 { Some(o) } else { None });| let cond_snip = utils::snippet(cx, cond.span, "[condition]"); | ||
| let arg_snip = utils::snippet(cx, then_arg.span, ""); | ||
| let help = format!( | ||
| "consider using `bool::then` like: `{}.then(|| {{ /* snippet */ {} }})`", |
There was a problem hiding this comment.
If then block has no statements, it would be better to remove the placeholder like /* snippet */.
giraffate
added
S-waiting-on-author
|
@giraffate
Please take a look again :) |
|
@bors r+ It looks good. Thanks! |
|
📌 Commit 11d2af7 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
| /// 42 | ||
| /// }); | ||
| /// ``` | ||
| pub IF_THEN_SOME_ELSE_NONE, |
There was a problem hiding this comment.
Since the MSRV feature is pretty new an we don't have any automated checks for it yet:
If an MSRV is added to a lint, the lint has also to be added in the config option. I wrote documentation about this in #6977. cc @rust-lang/clippy
There was a problem hiding this comment.
I will prepare a fix for this (and possible other lints soon.
There was a problem hiding this comment.
If an MSRV is added to a lint, the lint has also to be added in the config option.
Oh I didn't know that. I'll keep that in mind for the next time I implement a new lint.
I will prepare a fix for this (and possible other lints soon.
Thanks!
There was a problem hiding this comment.
Thanks for your follow-up!
There was a problem hiding this comment.
Oh I didn't know that. I'll keep that in mind for the next time I implement a new lint.
No worries, this isn't/wasn't documented anywhere, so that's on me for missing it while reviewing the documentation.
…, r=flip1995 Add a missing lint to MSRV config doc A follow-up of #6859. changelog: Add a missing lint to MSRV config doc
Resolves #6760
changelog: Added a new lint:
if_then_some_else_none