Skip to content

Conversation

@qmwrygdxpzc
Copy link

@qmwrygdxpzc qmwrygdxpzc commented Sep 23, 2025

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 23, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 23, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@github-actions
Copy link

github-actions bot commented Sep 23, 2025

No changes for a84945f

Copy link
Member

@samueltardieu samueltardieu left a 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.

View changes since this review

@qmwrygdxpzc
Copy link
Author

In the example with the 2 functions, what should the linter do?

@samueltardieu
Copy link
Member

In the example with the 2 functions, what should the linter do?

Probably nothing.

@qmwrygdxpzc
Copy link
Author

It has been about 2 weeks. Can someone take a look?

@qmwrygdxpzc
Copy link
Author

@samueltardieu Now there are some less trivial tests involving macros. Please take another look.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 13, 2025
@rustbot rustbot removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) has-merge-commits PR has merge commits, merge with caution. labels Oct 13, 2025
@qmwrygdxpzc
Copy link
Author

I used the reindent_multiline function because the check that I used as an example has it. I noticed that some checks don't use it. When should it be used?

@rustbot

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
@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2025

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.

@qmwrygdxpzc
Copy link
Author

I used the reindent_multiline function because the check that I used as an example has it. I noticed that some checks don't use it. When should it be used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-fcp S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

5 participants