Enable calling context on Option<T> by adding an OptionExt trait.

[marmistrz]

No description provided.

[otavio]

I am not sure I like the idea of treating Option<T> as Result<T, E> as this is not the purpose of it. Would you mind to explain your motivation behind it?

[marmistrz]

Often when Option<T> is returned, None means an error. For example https://doc.rust-lang.org/std/path/struct.Path.html#method.to_str

Then one would have to be very verbose by:

fn foo(...) -> Fallible<()> {
    let path: Path = ....;
    if let Some(x) = path.to_str() {
         ....
    } else {
         return Err(format_err!("path is not valid utf-8"));
    }
...
}

Here, using context makes it really concise:

let x = path.to_str.context("path is not valid utf-8")?;

another use case: I have a struct:

struct Session {
    session_id: String,
    node_id: NodeId,
}

pub struct SessionMan {
    hub_ip: SocketAddr,
    session: Option<Session>,
}

where None means there's no session established yet. The code to be written is exactly as in the Path example.

[otavio]

Good, I understand your motivation. My question now is if it makes sense to use Option<T> if None is an error. Instead, it seems it should use Result<T, failure::Error> and then return the Error when None would be used.

When I see Option<T> it communicates to me that None is valid. If it is not, I think we should not use Option<T>. Maybe I am missing something?

[marmistrz]

When it comes to https://doc.rust-lang.org/std/path/struct.Path.html#method.to_str , please ask the Rust development team :) std::path::Path is using Options all along

When it comes to my usecase, yes, Option is exactly what I need. There is either a session or there is no session. If a user tries to execute an action, which requires an existing session, I need to report a human-readable error. Result is not an option here, since until a call is made, there's no error, so self.session cannot be a Result

[Celti]

I think perhaps the main contention here comes from the fact that you're using the same method name to turn an Option into a Result that you use to add context to an already-existing Result.

Perhaps the method should be named ok_or_context?

[marmistrz]

Ok, I'll change that to ok_or_context

[marmistrz]

Is there anything that prevents this from being merged?

[mitsuhiko]

I'm not entirely opposed but also not fully convinced yet that this is a good idea. Does anyone else have opinions on this?

[Celti]

I'd be very happy with it, I've run into the same verbosity issue many times, and the ability to quickly and conveniently say "This why None is an error" is highly necessary.

[davidbarsky]

I'm in favor of this change, or something that looks a lot like this. This is something that I've run into often.

[ssendev]

I too think this should exist (in fact i started writing a pull request before seeing it's already here).
my reasoning for why this should exist (and be named context) is that it makes the interface more uniform since it means foo? can always be replaced with foo.context("foo")? and ? is already converting Option to Result so if context does it too it's not really something new.

[pierzchalski]

Just adding my experience that yes, this comes up a fair amount in practice when you're calling a function for which None is a valid outcome for the function but is an error for you, the caller. Also, aside from the awkward naming I don't actually see why ResultExt<T, ()> couldn't be implemented for Option<T> (in preparation for a future when Try is stable).

The alternative is the slightly verbose maybe_foo().ok_or(()).context("whatever")?, relying on () being a valid type argument for ResultExt::context.

[marmistrz]

Is there anything that prevents this change from being merged?

[marmistrz]

Since there is no response from the maintainers, I created a crate which provides ok_or_context for Option (and, as an opt-in feature, context for futures::Future)
https://github.com/marmistrz/failure_ext
https://crates.io/crates/failure_ext