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.

Sign up for GitHub

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

Jump to bottom

Stabilize Result::map_or_else #66322

Merged
merged 2 commits into from
Nov 24, 2019
Merged

Stabilize Result::map_or_else #66322

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2de6c35
make Result::map_or_else consistent with Option::map_or_else
tesuji Nov 12, 2019
c06a8ea
stabilize Result::map_or_else
tesuji Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/libcore/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,6 @@ impl<T, E> Result<T, E> {
/// Basic usage:
///
/// ```
/// #![feature(result_map_or_else)]
/// let k = 21;
///
/// let x : Result<_, &str> = Ok("foo");
Expand All @@ -539,9 +538,12 @@ impl<T, E> Result<T, E> {
/// assert_eq!(x.map_or_else(|e| k * 2, |v| v.len()), 42);
/// ```
#[inline]
#[unstable(feature = "result_map_or_else", issue = "53268")]
pub fn map_or_else<U, M: FnOnce(T) -> U, F: FnOnce(E) -> U>(self, fallback: F, map: M) -> U {
self.map(map).unwrap_or_else(fallback)
Copy link
Contributor

Choose a reason for hiding this comment

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

What was wrong with the previous implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new implementation is consistent with Option::map_or_else: https://doc.rust-lang.org/nightly/src/core/option.rs.html#491-496

So we are following Principle of Least Surprise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even less Surprising would be to not change an implementation in a stabilization PR.

Also, IMO the original implementation was better because it more clearly shows the relation between map_or_else, map, and unwrap_or_else. The whole point of these methods is that we don't have to write matches everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you like it, we could send a PR later to fix it in both Option and Result.

Copy link

Choose a reason for hiding this comment

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

@timvermeulen That might be true, but following the paradigm of keeping things simple and consistent is also needed. The original idea was to create map_or_else and unwrap_or_else the same as on Option, and then things get switched around. That is just absurd, and makes it harder to deal with because you to constantly ask yourself, "which way arround was this command?!" - and honestly i dont have time for that in a progeramming language.

Copy link

Choose a reason for hiding this comment

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

@tomwhoiscontrary I think that question has been asked before, or a derivative thereof. The conclusion was that Options map_or_else is already in stable and would therefore not be changed, as that would be breaking much more.
Part of the discussion is in here #53268

Copy link
Contributor

Choose a reason for hiding this comment

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

@KatsuoRyuu My question is the one i asked - why is Option::map_or_else the way it is. I am not suggesting changing it. I want to understand why it is the way it is.

Copy link

Choose a reason for hiding this comment

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

@tomwhoiscontrary sorry for the misunderstanding that, will see if I can find the documentation for that for you, I think I saw it somewhere ;)

Copy link

@KatsuoRyuu KatsuoRyuu Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@tomwhoiscontrary this is the thread I remembered seeing, rust-lang/rfcs#1025. So the reason for the initially is to follow the convention of map_or, so all parameters are always prepended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! The explanation makes sense. If we had keyword arguments, this problem would disappear!

#[stable(feature = "result_map_or_else", since = "1.41.0")]
pub fn map_or_else<U, D: FnOnce(E) -> U, F: FnOnce(T) -> U>(self, default: D, f: F) -> U {
match self {
Ok(t) => f(t),
Err(e) => default(e),
}
}

/// Maps a `Result<T, E>` to `Result<T, F>` by applying a function to a
Expand Down