Skip to content
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

Add the convenience method Option::is_some_and #75298

Conversation

LukasKalbertodt
Copy link
Member

Adds:

impl Option<T> {
    fn is_some_and<P>(&self, predicate: P) -> bool
    where
        P: FnOnce(&T) -> bool;
}

I recently got the idea for this method. It solves a "pain point" (albeit a small one) I encountered many times over the years: checking if an Option is Some(x) and if a condition holds for x. Currently possible solutions:

  • Verbose:

    if let Some(s) = opt {
        if s.is_ascii() {
            // body
        }
    }
    match opt {
        Some(s) if s.is_ascii() => {
            // body
        }
        _ => {}
    }
  • Concise, but the unwrap is not nice :/

    if opt.is_some() && opt.unwrap().is_ascii() {
        // body
    }
  • Somewhat concise, but not easy to read. At least I have to actively think about it for a few seconds before I know what exactly it does.

    if opt.map(|s| s.is_ascii()).unwrap_or(false) {
        // body
    }
  • Concise and probably the best solution currently. However, uses a macro and the "reading flow" is not optimal (due to the two ifs).

    if matches!(opt, Some(s) if s.is_ascii()) {
        // body
    }

With the new method, it would look like this:

if opt.is_some_and(|s| s.is_ascii()) {
    // body
}

I think this is a bit better than the matches! version. However, I understand there are a bunch of reasons not to add this method:

  • Option already has lots of helper methods and expanding the API surface has some disadvantages. If the advantage over the matches! solution is too small, adding is_some_and might not be worth it.
  • Sometimes, people might want the opposite: true in the None case. If that use case is equally as common (it is not in my experience), there should be a method for that, too. Something like is_none_or. But adding two methods is even worse than one, so it might not be worth it.
  • and in the name might be confusing because of its use in Option::and and Option::and_then.

So I can totally understand if we decide against this addition. Just wanted to hear people's opinion on this :)

I tried searching for previous attempts to add such a method, but didn't find anything. If I missed something, please let me know.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2020
@the8472
Copy link
Member

the8472 commented Aug 8, 2020

Isn't that essentially doing what if let chains (#53667) aim to handle?

@Mark-Simulacrum
Copy link
Member

I usually do this with map_or(false, |s| s.is_ascii()). I think a dedicated method might be nice but could also just be extra confusion - the if let and if pattern I find is often necessary anyway if I want any sort of control flow as part of the condition (in particular, an await).

@qnighy
Copy link
Contributor

qnighy commented Aug 14, 2020

How about adding the de Morgan dual is_none_or too?

@withoutboats
Copy link
Contributor

withoutboats commented Aug 14, 2020

I'm inclined against adding this method.

There are already many combinators on Option; learning the signatures of each of them is a part of becoming an effective Rust programmer, and the more we add the more complex that becomes (superlinearly).

When I need to write this, I also use the opt.map_or(false, |s| s.is_ascii()) pattern. The fact that an existing combinator so neatly already supports this as a pattern makes the value of this combinator lower in my mind as well. The matches-based pattern is neat too.

(Not that it changes my opinion about adding it, but if we were to add this I would definitely expect it to have the signature (self, FnOnce(T) -> bool), rather than by reference. Users who need it by-reference should call as_ref first.)

@Dylan-DPC-zz
Copy link

Going by the reviewer's last comment, there is no motivation to adding this. so i'm closing this pull request. Thanks

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked-closed and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2020
@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked-closed labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants