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

Tracking issue for Option::filter (feature `option_filter`) #45860

Closed
dtolnay opened this Issue Nov 8, 2017 · 17 comments

Comments

Projects
None yet
10 participants
@dtolnay
Member

dtolnay commented Nov 8, 2017

This is a tracking issue for Option::filter.

Unresolved questions:

  • decide on name (filter vs and_if vs …)
@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Nov 8, 2017

Contributor

@dtolnay Could you add a task "decide on the name (filter() vs. and_then() vs. ???)" to your comment? Also: the PR is ready now.

Contributor

LukasKalbertodt commented Nov 8, 2017

@dtolnay Could you add a task "decide on the name (filter() vs. and_then() vs. ???)" to your comment? Also: the PR is ready now.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Nov 8, 2017

@dtolnay

This comment has been minimized.

Show comment
Hide comment
@dtolnay

dtolnay Nov 8, 2017

Member

I think and_if is a brilliant name. Really great idea and strong contender. At this point I would still lean toward filter.

I find that the and/or methods on Option make perfect sense and the names reflect what the method does, but it still takes me multiple seconds when I see something like or_else or map_or to reverse engineer what that means in terms of argument types and return types and pattern matching and behavior of my code.

In contrast, filter is strongly immediately associated with taking a filter function &T -> bool and using it to decide whether to keep the values in self. A short name with that sort of immediate strong association was not available for the things the and/or family of methods do, or they would have been called something different.

Member

dtolnay commented Nov 8, 2017

I think and_if is a brilliant name. Really great idea and strong contender. At this point I would still lean toward filter.

I find that the and/or methods on Option make perfect sense and the names reflect what the method does, but it still takes me multiple seconds when I see something like or_else or map_or to reverse engineer what that means in terms of argument types and return types and pattern matching and behavior of my code.

In contrast, filter is strongly immediately associated with taking a filter function &T -> bool and using it to decide whether to keep the values in self. A short name with that sort of immediate strong association was not available for the things the and/or family of methods do, or they would have been called something different.

kennytm added a commit to kennytm/rust that referenced this issue Nov 10, 2017

Rollup merge of rust-lang#45863 - LukasKalbertodt:add-option-filter, …
…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?

niklasf added a commit to niklasf/rust-chessground that referenced this issue Nov 13, 2017

@nvzqz

This comment has been minimized.

Show comment
Hide comment
@nvzqz

nvzqz Nov 16, 2017

Contributor

As I mentioned in #45863 (comment), I have a question regarding the signature of this method:

Wouldn't this method be more flexible if the FnOnce took &mut T?

My suggested change would differ from Vec::retain, which this method acts similarly to (except not in-place). Perhaps it would be worth considering changing the signature of Vec::retain too?

I understand that this method is essentially shorthand for option.iter().filter(predicate).next(). My suggested change would be equivalent to option.iter_mut().filter(predicate).next().

Contributor

nvzqz commented Nov 16, 2017

As I mentioned in #45863 (comment), I have a question regarding the signature of this method:

Wouldn't this method be more flexible if the FnOnce took &mut T?

My suggested change would differ from Vec::retain, which this method acts similarly to (except not in-place). Perhaps it would be worth considering changing the signature of Vec::retain too?

I understand that this method is essentially shorthand for option.iter().filter(predicate).next(). My suggested change would be equivalent to option.iter_mut().filter(predicate).next().

@netvl

This comment has been minimized.

Show comment
Hide comment
@netvl

netvl Jan 7, 2018

Contributor

+1 for filter() - this also has precedents in other languages:

Contributor

netvl commented Jan 7, 2018

+1 for filter() - this also has precedents in other languages:

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Jan 21, 2018

Contributor

@nvzqz Do you have a specific use case in mind? Yes, the method would be more flexible with FnOnce(&mut T) -> bool as predicate, but we don't necessarily want this flexibility. Simple example: Rust variables are immutable by default in order to limit what you can do -- more flexibility/possibilities are not always a good thing.

So without good use case in mind, I would keep the signature as is: I don't think anyone would expect a predicate of a method called filter() to change the inner value.

Contributor

LukasKalbertodt commented Jan 21, 2018

@nvzqz Do you have a specific use case in mind? Yes, the method would be more flexible with FnOnce(&mut T) -> bool as predicate, but we don't necessarily want this flexibility. Simple example: Rust variables are immutable by default in order to limit what you can do -- more flexibility/possibilities are not always a good thing.

So without good use case in mind, I would keep the signature as is: I don't think anyone would expect a predicate of a method called filter() to change the inner value.

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Jan 21, 2018

Contributor

About the name: please note that the RFC thread already contains quite a few arguments for and against certain names. I'll try to summarize the most important points here:


The two competing names are filter and and_if. One comment on their differences:

In other words, .and_if describes how it does X, .filter describes what X is. This is the distinction between imperative and declarative programming where the .filter is declarative.

Comparing the two:

AdvantagesDisadvantages
filter
  • Same as in most other languages
  • Same as in Iterator
and_if
  • Fits other methods of Option more closely
  • More descriptive in the context of control flow

How this method is called in other languages:

  • Haskell: mfilter
  • Scala: filter
  • C++17: no such method (right?)
  • boost::optional: no such method (right?)
  • Idris: no such method (right?)
  • Nim: filter
  • Java8: filter
  • Swift: no such method (right?)
  • Python: filter (kind of)
Contributor

LukasKalbertodt commented Jan 21, 2018

About the name: please note that the RFC thread already contains quite a few arguments for and against certain names. I'll try to summarize the most important points here:


The two competing names are filter and and_if. One comment on their differences:

In other words, .and_if describes how it does X, .filter describes what X is. This is the distinction between imperative and declarative programming where the .filter is declarative.

Comparing the two:

AdvantagesDisadvantages
filter
  • Same as in most other languages
  • Same as in Iterator
and_if
  • Fits other methods of Option more closely
  • More descriptive in the context of control flow

How this method is called in other languages:

  • Haskell: mfilter
  • Scala: filter
  • C++17: no such method (right?)
  • boost::optional: no such method (right?)
  • Idris: no such method (right?)
  • Nim: filter
  • Java8: filter
  • Swift: no such method (right?)
  • Python: filter (kind of)
@KodrAus

This comment has been minimized.

Show comment
Hide comment
@KodrAus

KodrAus Feb 26, 2018

It looks like we're still in the bikeshedding phase here, so just leaving a note to say I found my way here after typing .filter automatically and discovering the method exists and is unstable :)

KodrAus commented Feb 26, 2018

It looks like we're still in the bikeshedding phase here, so just leaving a note to say I found my way here after typing .filter automatically and discovering the method exists and is unstable :)

@bb010g

This comment has been minimized.

Show comment
Hide comment
@bb010g

bb010g Feb 28, 2018

@LukasKalbertodt FnOnce(&mut T) -> bool would be useful when dealing with data structures that cache by mutating themselves on otherwise normal accessors. (Although you could cover this in some cases with a Cell.) Could this be implemented in a separate Option::filter_mut?

bb010g commented Feb 28, 2018

@LukasKalbertodt FnOnce(&mut T) -> bool would be useful when dealing with data structures that cache by mutating themselves on otherwise normal accessors. (Although you could cover this in some cases with a Cell.) Could this be implemented in a separate Option::filter_mut?

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Feb 28, 2018

Contributor

@bb010g Good point. However, I think I still tend to keep the closure as it is right now. A few reasons:

  • The closure of Iterator::filter also takes immutable references. I guess your argument would also apply to that function, but for for the sake of consistency I'd use the same signature for Option::filter.
  • Maybe a data structure as you proposed it should really use cells inside such that its API reflects the logical mutability-properties?
  • I can't quite imagine that one would call filter on an Option<DataStructure> directly: if the closure returns false, we throw away the entire data structure.

However, I'm not quite sure and don't have a very strong opinion on this decision.

Contributor

LukasKalbertodt commented Feb 28, 2018

@bb010g Good point. However, I think I still tend to keep the closure as it is right now. A few reasons:

  • The closure of Iterator::filter also takes immutable references. I guess your argument would also apply to that function, but for for the sake of consistency I'd use the same signature for Option::filter.
  • Maybe a data structure as you proposed it should really use cells inside such that its API reflects the logical mutability-properties?
  • I can't quite imagine that one would call filter on an Option<DataStructure> directly: if the closure returns false, we throw away the entire data structure.

However, I'm not quite sure and don't have a very strong opinion on this decision.

@bb010g

This comment has been minimized.

Show comment
Hide comment
@bb010g

bb010g Feb 28, 2018

You'd probably want filter_mut for consistency with Iterator. You can't always use Cell due to the Copy constraint. As for dropping the structure, this may be desirable when working with types like Arc a lot in a concurrent environment.

bb010g commented Feb 28, 2018

You'd probably want filter_mut for consistency with Iterator. You can't always use Cell due to the Copy constraint. As for dropping the structure, this may be desirable when working with types like Arc a lot in a concurrent environment.

@LukasKalbertodt

This comment has been minimized.

Show comment
Hide comment
@LukasKalbertodt

LukasKalbertodt Feb 28, 2018

Contributor

So I guess we keep this issue just for filter. Adding filter_mut or something similar can then be proposed via another RFC, I'd say.

Then there's only the naming issue left. Here again, I don't have a strong opinion. I like both names a lot and would be happy either way.

Contributor

LukasKalbertodt commented Feb 28, 2018

So I guess we keep this issue just for filter. Adding filter_mut or something similar can then be proposed via another RFC, I'd say.

Then there's only the naming issue left. Here again, I don't have a strong opinion. I like both names a lot and would be happy either way.

@Emerentius

This comment has been minimized.

Show comment
Hide comment
@Emerentius

Emerentius Mar 7, 2018

Contributor

There are not very many and_* functions. It's only and and and_then and one of those was a poor choice. and is fitting in the logical sense of the word. and_then loses that meaning and is purely temporal. It could have just as well been used for map. The proper name for and_then is flat_map.
The and_if name evokes no meaning in my head. And if X then what? The name works only with convention but that convention doesn't exist. filter is well established already. We would need good reasons to deviate from this.

There are a bunch of *_or* variants of other functions and for all of them it's immediately obvious what they do and what kind of data they expect from the operation they extend.

Contributor

Emerentius commented Mar 7, 2018

There are not very many and_* functions. It's only and and and_then and one of those was a poor choice. and is fitting in the logical sense of the word. and_then loses that meaning and is purely temporal. It could have just as well been used for map. The proper name for and_then is flat_map.
The and_if name evokes no meaning in my head. And if X then what? The name works only with convention but that convention doesn't exist. filter is well established already. We would need good reasons to deviate from this.

There are a bunch of *_or* variants of other functions and for all of them it's immediately obvious what they do and what kind of data they expect from the operation they extend.

@0x7CFE 0x7CFE referenced this issue Mar 12, 2018

Merged

New Transaction Queue implementation #8074

9 of 10 tasks complete
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 17, 2018

Contributor

the RFC thread already contains quite a few arguments for and against certain names

As it seems unlikely that more time will produce new arguments either way, I’d like to propose stabilizing this feature as it currently is in Nightly.

@rfcbot fcp merge

Contributor

SimonSapin commented Mar 17, 2018

the RFC thread already contains quite a few arguments for and against certain names

As it seems unlikely that more time will produce new arguments either way, I’d like to propose stabilizing this feature as it currently is in Nightly.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Mar 17, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Mar 17, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Mar 19, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Mar 19, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Mar 29, 2018

The final comment period is now complete.

rfcbot commented Mar 29, 2018

The final comment period is now complete.

tmccombs added a commit to tmccombs/rust that referenced this issue Apr 2, 2018

kennytm added a commit to kennytm/rust that referenced this issue Apr 11, 2018

Rollup merge of rust-lang#49575 - tmccombs:option-filter-stabilize, r…
…=withoutboats

Stabilize `Option::filter`.

Fixes rust-lang#45860

@bors bors closed this in #49575 Apr 12, 2018

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