-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add a check for some followed by filter #15745
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
base: master
Are you sure you want to change the base?
Conversation
No changes for edd2d3b |
The program will suggest using the `then_some` method. changelog: [`filter_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some
4746c00
to
d34f4ed
Compare
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.
Thanks for your contribution. However, the lint cannot be considered for merging as-is:
- It should include autofixes suggestions when possible.
- It should cover more complex cases.
For example, with the following code
fn with_side_effect(x: u32) -> u32 {
println!("In `with_side_effect()`");
x * 10 + 30
}
fn f(x: u32) -> Option<u32> {
Some(with_side_effect(x)).filter(|&x| x > 40)
}
the lint suggests using then_some()
inside f()
. However, I don't see what would be the replacement code here.
You should include tests for the most complicated situations you can think of, as the lint should not fail, don't just cover the simplest case where it is obvious it will work. For example, what is part of the expression comes from a macro? Also, bool::then_some()
has been introduced in Rust 1.62.0, so the MSRV must be checked as well.
Hope this review will help you go forward.
In the example with the 2 functions, what should the linter do? |
Probably nothing. |
The situation where the condition depends on the value is excluded form linting. Now the program always fixes automatically. Tests are added for code involving macros. The MSRV is checked.
It has been about 2 weeks. Can someone take a look? |
Add a check for some followed by filter.
The program will suggest using the
then_some
method.changelog: [
filter_some
]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some