Make Result a wrapper, not a re-implementation.
#2
Conversation
| /// Construct a new [`propagate::Result`][crate::Result] with the given success value. | ||
| #[inline] | ||
| pub fn ok<T, E, S>(ok_value: T) -> Result<T, E, S> { | ||
| self::Result(Ok(ok_value)) | ||
| } | ||
|
|
||
| /// Construct a new [`propagate::Result`][crate::Result] with the given error value. | ||
| #[inline] | ||
| #[track_caller] | ||
| pub fn err<T, E, S: Traced + Default, F: From<E>>(err_value: E) -> Result<T, F, S> { | ||
| let wrapped = TracedError::new(F::from(err_value)); | ||
| self::Result(Err(wrapped)) | ||
| } |
There was a problem hiding this comment.
Maybe macros instead? Or make them associated functions on the Result struct?
| Ok(Some(x)) => Some(Ok(x)), | ||
| Ok(None) => None, | ||
| Err(e) => Some(Err(e)), | ||
| pub fn unpack(self) -> (std::result::Result<T, E>, Option<S>) { |
There was a problem hiding this comment.
Need help coming up with a good name for this operation.
unwrap is descriptive, but it is not a good choice because it means something totally different than {Result, Option}::unwrap().
unpack is descriptive, but to me it's not different enough from unwrap.
get is not very descriptive, but it isn't easily confusable with other operations.
There was a problem hiding this comment.
unpack seems okay to me.
Some alternatives I can think of
- unstack
- extract
- take_stack
- split_stack
- decompose
none of these are really better though, :/
| impl<T, E> Result<T, E> { | ||
| ///////////////////////////////////////////////////////////////////////// | ||
| // Querying the contained values | ||
| ///////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
If needed, we can add back in certain bits of functionality from std::result::Result. For example, having the following methods on propagate::Result could prove useful:
map_err()err()ok()unwrap()unwrap_or_else()
Point being, nothing about this change prevents us from making the interface of propagate::Result closer to std::result::Result down the line if we feel that it is called for.
I'll spend some time testing the ergonomics of propagate::Result at work over the coming weeks to see which of these functions it wants to have.
|
@yaahc thoughts on this proposal? |
Previously, `propagate::Result` was mostly a copy of
`std::result::Result`, with a different `Err` type and modified
`FromResidual` implementations.
Because of this, users would have to to use
`propagate::Result::{Ok, Err}` to construct results (unless they used
`try blocks`).
This changes `propagate::Result<T, E, S>` to be a minimal wrapper around
a `std::result::Result<T, TracedError<E, S>>`. If users want to get to
the wrapped result, they can call `.unpack()`, which will also return
the traced stack if there is one.
With this change, users no longer have to (nor have the ability to)
shadow the standard library's `Ok` and `Err`.
Now, there should be much less confusion about what a
`propagate::Result` really is. It no longer pretends to be the same
thing as a `std::result::Result`.
|
Sorry it took so long to review this one. First off just want to make sure, is this PR still relevant? I know you've made a bunch of updates recently so I want to make sure I'm not reviewing the a PR that has been superseded by more recent changes. As for the changes themselves, they look good. The one thing I'm worried about is |
|
Nah sorry, this PR quickly became mostly irrelevant after I made the change from From my memory, I'm pretty sure the |
Previously,
propagate::Resultwas mostly a copy ofstd::result::Result, with a differentErrtype and modifiedFromResidualimplementations.Because of this, users would have to to use
propagate::Result::{Ok, Err}to construct results (unless they usedtryblocks).This changes
propagate::Result<T, E, S>to be a minimal wrapper around astd::result::Result<T, TracedError<E, S>>. If users want to get to the wrapped result, they can call.unpack(), which will also return the traced stack if there is one.With this change, users no longer need to (nor have the ability to) shadow the standard library's
OkandErr.Now, there should be much less confusion about what a
propagate::Resultreally is. It no longer pretends to be the samething as a
std::result::Result.