Skip to content

Conversation

LukasKalbertodt
Copy link
Contributor

@LukasKalbertodt LukasKalbertodt commented Nov 8, 2017

(old PR: #44996)

This is the implementation of RFC "Add Option::filter to the standard library". Tracking issue: #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?

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 8, 2017
@dtolnay dtolnay self-assigned this Nov 8, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the "Questions for code reviewers"! Everything looks good to me.

@dtolnay
Copy link
Member

dtolnay commented Nov 8, 2017

@rust-lang/libs this is adding an unstable Option::filter with the signature:

fn filter<P>(self, predicate: P) -> Self
where
    P: FnOnce(&T) -> bool

@dtolnay
Copy link
Member

dtolnay commented Nov 8, 2017

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 8, 2017

📌 Commit e652144 has been approved by dtolnay

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2017
kennytm added a commit to kennytm/rust that referenced this pull request Nov 10, 2017
…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?
bors added a commit that referenced this pull request Nov 10, 2017
Rollup of 9 pull requests

- Successful merges: #45783, #45856, #45863, #45869, #45878, #45882, #45887, #45895, #45901
- Failed merges:
@bors bors merged commit e652144 into rust-lang:master Nov 10, 2017
@nvzqz
Copy link
Contributor

nvzqz commented Nov 15, 2017

I haven't been following the discussion on this closely but wouldn't this method be more flexible if the FnOnce took &mut T?

Then again, this method acts similarly to Vec::retain (except not in-place), which passes &T too.

@LukasKalbertodt
Copy link
Contributor Author

@nvzqz please mention concerns or suggestions like this in the tracking issue to have all discussion in one place. Thanks :)

@LukasKalbertodt LukasKalbertodt deleted the add-option-filter branch May 3, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants