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

Stabilize core::convert::identity #57322

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
5 participants
@Centril
Copy link
Contributor

Centril commented Jan 4, 2019

r? @SimonSapin

fixes #53500

This is waiting for FCP to complete but in the interim it would be good to review.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 5, 2019

r+, pending FCP completion

@@ -86,14 +84,13 @@
/// Using `identity` to keep the `Some` variants of an iterator of `Option<T>`:

This comment has been minimized.

@clarcharr

clarcharr Jan 5, 2019

Contributor

I feel like this particularly would be better as flatten with an explicit specialisation for Option, rather than filter_map.

This comment has been minimized.

@Centril

Centril Jan 5, 2019

Contributor

I disagree.

First, I think that .filter_map(identity) is clearer wrt. intent (and also semantics). I can clearly tell that filtering is happening because it says so in the name. I know that .filter_map takes f: T -> Option<U> and with identity I fix T = Option<U> and thus I get back all the Somes. The .flatten() method doesn't tell me any of this, in particular you cannot tell without knowing that Option is being operated on what the semantics are. Thus the reasoning footprint is greater with .flatten().

Second, even tho .flatten() is more general than join :: Monad m => m (m a) -> m a I think the primary use of the method should be for monadic join and using it for other purposes will break some people's minds (e.g. mine).

Third, I think that we shouldn't hold up stabilization on this issue; we can resolve it in a subsequent PR or issue if you feel strongly.

@Centril Centril added the T-libs label Jan 10, 2019

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 14, 2019

FCP has completed, and therefore:

@bors r=SimonSapin p=1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

📌 Commit e75dab7 has been approved by SimonSapin

Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019

Rollup merge of rust-lang#57322 - Centril:stabilize-identity, r=Simon…
…Sapin

Stabilize core::convert::identity

r? @SimonSapin

fixes rust-lang#53500

This is waiting for FCP to complete but in the interim it would be good to review.

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57590 - Centril:rollup, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #57322 (Stabilize core::convert::identity)
 - #57388 (use ignore directives for run-make tests)
 - #57477 (clarify resolve typo suggestion)
 - #57556 (privacy: Fix private-in-public check for existential types)
 - #57584 (Remove the `connect_timeout_unroutable` test.)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

⌛️ Testing commit e75dab7 with merge 1a3a3df...

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57322 - Centril:stabilize-identity, r=SimonSapin
Stabilize core::convert::identity

r? @SimonSapin

fixes #53500

This is waiting for FCP to complete but in the interim it would be good to review.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: SimonSapin
Pushing 1a3a3df to master...

@bors bors merged commit e75dab7 into rust-lang:master Jan 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Centril Centril deleted the Centril:stabilize-identity branch Jan 14, 2019

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