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

Stabilize Result::map_or_else #66322

Merged
merged 2 commits into from Nov 24, 2019

Conversation

@lzutao
Copy link
Contributor

lzutao commented Nov 12, 2019

Stabilized this API:

impl<T, E> Result<T, E> {
    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),
        }
    }
}

Closes #53268
r? @SimonSapin

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 12, 2019

This matches the signature of already-stable Option::map_or_else (except that the default callback gets the error value as a parameter)

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 12, 2019

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@@ -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)

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen Nov 12, 2019

Contributor

What was wrong with the previous implementation?

This comment has been minimized.

Copy link
@lzutao

lzutao Nov 12, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen Nov 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@lzutao

lzutao Nov 12, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@KatsuoRyuu

KatsuoRyuu Nov 13, 2019

@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.

This comment has been minimized.

Copy link
@KatsuoRyuu

KatsuoRyuu Nov 13, 2019

@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

This comment has been minimized.

Copy link
@tomwhoiscontrary

tomwhoiscontrary Nov 13, 2019

Contributor

@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.

This comment has been minimized.

Copy link
@KatsuoRyuu

KatsuoRyuu Nov 14, 2019

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

This comment has been minimized.

Copy link
@KatsuoRyuu

KatsuoRyuu Nov 14, 2019

@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.

This comment has been minimized.

Copy link
@tomwhoiscontrary

tomwhoiscontrary Nov 14, 2019

Contributor

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

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 12, 2019

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

@NilsIrl

This comment has been minimized.

Copy link

NilsIrl commented Nov 15, 2019

Shouldn't the arguments be the other way round to match those of Option?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 22, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@lzutao

This comment has been minimized.

Copy link
Contributor Author

lzutao commented Nov 23, 2019

@rust-highfive rust-highfive assigned dtolnay and unassigned SimonSapin Nov 23, 2019
Copy link
Member

dtolnay left a comment

Thanks!

Relatedly, I wish that we had more realistic examples for these type of combinator methods. The current example code is:

let k = 21;

let x : Result<_, &str> = Ok("foo");
assert_eq!(x.map_or_else(|e| k * 2, |v| v.len()), 3);

which is so contrived that it doesn't help you understand how the method is intended to be used.

It would be better to find real uses in the wild and distill them into example code. Something like this is where I would expect to encounter map_or_else:

    ...
    .map_or_else(Response::DatabaseError, |id| {
        if id.is_empty() {
            Response::Empty
        } else {
            Response::User(id.to_string())
        }
    })
@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Nov 24, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

📌 Commit c06a8ea has been approved by dtolnay

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

⌛️ Testing commit c06a8ea with merge 7d761fe...

bors added a commit that referenced this pull request Nov 24, 2019
Stabilize Result::map_or_else

Stabilized this API:
```rust
impl<T, E> Result<T, E> {
    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),
        }
    }
}
```

Closes #53268
r? @SimonSapin
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 24, 2019

☀️ Test successful - checks-azure
Approved by: dtolnay
Pushing 7d761fe to master...

@bors bors added the merged-by-bors label Nov 24, 2019
@bors bors merged commit c06a8ea into rust-lang:master Nov 24, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191112.8 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@lzutao lzutao deleted the lzutao:consistent-result-map_or_else branch Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.