- Notifications
You must be signed in to change notification settings - Fork 1.9k
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 a84945f |
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. |
| It has been about 2 weeks. Can someone take a look? |
| @samueltardieu Now there are some less trivial tests involving macros. Please take another look. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a74059f to 6f173cd Compare | I used the |
This comment has been minimized.
This comment has been minimized.
The program will suggest using the `then_some` method. changelog: [`filter_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some
74fbe8a to a84945f Compare | This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| I used the |
Add a check for some followed by filter.
The program will suggest using the
then_somemethod.changelog: [
filter_some]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_some