Skip to content

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
Contributor

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