Skip to content
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 Result::map_or_else #53268

Closed
Boscop opened this issue Aug 11, 2018 · 16 comments · Fixed by #66322
Closed

Tracking issue for Result::map_or_else #53268

Boscop opened this issue Aug 11, 2018 · 16 comments · Fixed by #66322
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Boscop
Copy link

Boscop commented Aug 11, 2018

Result should also have map_or_else(), like Option (but the closure for the else case should also get the Err as argument, like with unwrap_or_else).

Both Option and Result have unwrap_or_else and unwrap_or, so they should also both have map_or_else.

@frewsxcv frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 11, 2018
@withoutboats withoutboats added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Aug 29, 2018
@withoutboats withoutboats changed the title Result should also have map_or_else(), like Option Tracking issue for Result::map_or_else Aug 29, 2018
@withoutboats
Copy link
Contributor

Implemented in #53777

kennytm added a commit to kennytm/rust that referenced this issue Sep 12, 2018
…xcrichton

Implemented map_or_else for Result<T, E>

Fulfills rust-lang#53268
The example is ripped from `Option::map_or_else`, with the types corrected.
@liigo
Copy link
Contributor

liigo commented Sep 21, 2018

I think its parameters should be changed
from
fn map_or_else(self, fallback: F, map: M)
to
fn map_or_else(self, map: M, fallback: F)

@rpjohnst
Copy link
Contributor

It would also be good to have Result::map_or.

@Boscop
Copy link
Author

Boscop commented Sep 28, 2018

@liigo Yes, having the fallback as second argument makes more logical sense because it mirrors the if else structure, but this would be a breaking change now :(

@Emerentius
Copy link
Contributor

@Boscop That's not an issue because it's still unstable. In most cases the error is of a different type than the value and it would cause a compile time error, anyway.

@Boscop
Copy link
Author

Boscop commented Oct 15, 2018

@Emerentius I agree that having the ok handler as first arg would make more sense, but I'm not sure how we can change it now.. map_or_else's arg order was chosen to be consistent with map_or. We would have to change the order for both then. I guess you'd have to do a RFC to suggest changing this.

@Emerentius
Copy link
Contributor

Emerentius commented Oct 15, 2018

map_or was flipped from the obvious order so that the typically short default case would be in front of the potentially long closure. Breaking consistenty with that is less troublesome than the map_or_else precedent from Option which is stable, already.
Looks like we're stuck with it…

@traviscross
Copy link
Contributor

Look on the bright side. The original ergonomic argument that the typically longer closure should be at the end is still compelling and provides a reasonable basis for making consistent decisions about this sort of thing.

To second what was mentioned above, it would make a lot of sense to add Result::map_or at the same time.

@ivanbakel
Copy link
Contributor

Is there any reason this shouldn't just be the tracking issue for map_or as well? To me, it seems like they can fall under the same feature flag (even if it won't be as accurately named anymore).

@jethrogb
Copy link
Contributor

Seconding map_or request

@tesuji
Copy link
Contributor

tesuji commented Nov 11, 2019

#66292 is adding map_or. Feedback welcomed.

@KatsuoRyuu
Copy link

I'm sorry but this implementation of map_or_else is incredibly annoying as it differs from Option.
When the original issue says:

Result should also have map_or_else(), like Option (but the closure for the else case should also >get the Err as argument, like with unwrap_or_else).

Both Option and Result have unwrap_or_else and unwrap_or, so they should also both have >map_or_else.

and the parameters are then switched, is mildly said absurd! - keep things consistent!

@mathstuf
Copy link
Contributor

Oof. Indeed. Not only are they switched from Option, but they're backwards from the name too. I really hope it's not too late. Does it need a new feature gate name or is that considered "nightly problems" too?

@tesuji
Copy link
Contributor

tesuji commented Nov 12, 2019

Stabilization PR in #66322

@cdbattags
Copy link

Looks like this is released with 1.41 but the order flip is really quite a pain.

Any precedent for why error first?

@lo48576
Copy link
Contributor

lo48576 commented Jan 31, 2020

@cdbattags Seems described in the first comment #53268 (comment).
I think it is "ok second" rather than "error first", similar to Option::map_or_else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.