-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Add Option::filter
to the standard library
#2124
Conversation
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.
Just some minor nitpick ^_^
One minor downside: including |
In all fairness, |
Could add Or is this kind of function aliasing discouraged in Rust? It's pretty common in languages like Ruby, e.g. |
There's been at least a few times that I've started to type |
I like this idea. If this is done then it seems like EDIT: Thought about it a bit more and I guess in the |
@Ryan1729 I thought about a fn parse(input: &str) -> Result<Ast, Error> {}
fn generate(ast: Ast) -> Result<String, Error> {}
parse(input)
.and_then(generate) Now I wanted to add another step which verifies the AST. It doesn't produce a result, but can produce an error. So the signature looks like this: fn check(ast: &Ast) -> Result<(), Error> {} Today, I think there is no better way to integrate it into my code than: parse(input)
.and_then(|ast| {
check(&ast).map(|_| ast)
})
.and_then(generate) So that's not particularly nice, but maybe ok-ish. I think I encountered this particular situation not nearly as often as situations in which |
@LukasKalbertodt If let string: Result<String, Error> = catch {
let ast = parse(input)?;
check(&ast)?;
generate(ast)?
}; |
Would it make sense to call it
|
Haskell calls this method I guess |
I agree with the |
I tried to find the name for this method in other languages:
(if you comment with information about other languages, I will add those here) Comparing both suggestions (with all points I could come up with):
(again, if you raise other points below, I'll add them) |
@LukasKalbertodt In other words, |
We'd probably prefer "retain" over "filter" for consistency with other containers like |
@glaebhoerl Isn't |
Hmm, could be. But my assumption is that the reason it wasn't named "filter" is the ambiguity around whether that means "filter-in" or "filter-out", which applies equally here. (For the record, I find the name "retain" kind of awkward and might prefer "filter" myself despite the ambiguity, in a vacuum, but again, consistency.) @kennytm, the downvoting of comments without explanation feels kind of obnoxious to me, could you please write a comment instead if you have a disagreement? Thank you. |
@glaebhoerl A method/function named I think we should pick the function name most people would intuitively just write without checking any docs, and in this respect I think @LukasKalbertodt The general function for filtering iterables is called "filter" in python. While this is not the same as |
I agree in that personally I think "filter" should be taken to mean filter-in, but I'm also not a representative sample, because I first learned Does anyone know what the motivation behind the name |
@glaebhoerl I think
in that twitter feed is right. There might be a few anecdotal examples - but are they, put together, notable? Haskell, is hardly alone in defining filter as filter-in. Other languages where it is filter-in: There are a bunch of other languages where it is called I'd like an example of a language where it is filter-out. |
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 the RFC! This is a small enough addition that I think we can go ahead with an implementation. Please open a tracking issue in rust-lang/rust and send a PR containing the implementation, fully documented with rustdoc, and an #[unstable] attribute linking to the tracking issue -- see rust-lang/rust#44682 for an example.
@dtolnay see rust-lang/rust#44996, which has been closed on hold until further discussion of the RFC:
|
@dtolnay As much as I'd like this RFC to be accepted immediately, I have to say: I don't think the name debate is settled, really. There are a few alternative names and I think Or do we want to discuss the naming issue after implementing and before stabilizing it? |
Thanks folks! It appears I was biased by landing on this RFC after trying to write In my experience we discuss naming before merging a new API, but only enough so that we don't get it blatantly wrong. Then we debate it more rigorously as part of FCP to stabilize. I think the logic there is that 1 month spent writing code against an API is more valuable toward deciding whether it is named correctly, compared to 1 month spent academizing about the name without writing code against it. One recent example of this is Other than the name, it seems like there is agreement that we want this functionality and the method signature in the RFC is the correct one. Is that your impression as well? @LukasKalbertodt do you feel that there could be strong arguments in favor of either name that haven't been voiced yet? Based on the existing arguments, what name are you leaning toward now and why? |
Yip.
I don't expect any more super important arguments, to be honest. And about my preference: I'm really not sure! I thought I'm absolutely fine with merging the RFC now and discussing the name on the tracking issue :) |
Sounds good to me! |
…r=dtolnay Add `Option::filter()` according to RFC 2124 (*old PR: rust-lang#44996) This is the implementation of [RFC "Add `Option::filter` to the standard library"](rust-lang/rfcs#2124). Tracking issue: rust-lang#45860 **Questions for code reviewers:** - Is the documentation sufficiently long? - Is the documentation easy enough to understand? - Is the position of the new method (after `and_then()`) a good one?
@LukasKalbertodt the rendered link is now broken, can you fix it? Thanks! |
Add the method
Option::filter<P>(self, predicate: P) -> Option<T>
to the standard library. This method makes it possible to easily throw away aSome
value depending on a given predicate. The callopt.filter(p)
is equivalent toopt.into_iter().filter(p).next()
.Rendered
I certainly know that there are plenty of active, more important RFCs. But since this RFC only introduces a tiny change to the standard library, I figured I should try writing it.
Fixes #1485