Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

Add Iterator over causes of a Fail #54

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

juggle-tux
Copy link
Contributor

this way you can look if a Fail contains e.g. an io::Error like this

if let Some(io_err) fail.causes().filter_map(Fail::downcast_ref::<io::Error>).next() {
    println!("we got an io::Error: {}" , io_err)
}

or even

impl Fail
    /// Returns the first apperence of a `Fail` ot type `F` in the chain of causes
    pub fn contains<F: Fail>(&self) -> Option<&F> {
        if let Some(fail) = self.downcast_ref::<F>() {
            Some(fail)
        } else {
            self.causes().filter_map(Fail::downcast_ref::<F>).next()
            
        }
    }
}

@withoutboats
Copy link
Contributor

Thanks for the PR! This was discussed in #41 and I'm mulling it over still.

A big question I think is whether or not this should include self as the first cause. root_cause for example does consider self to be a valid root cause.

I am not certain that &'a Fail should implement IntoIterator. This seems weird to me:

for cause in &err {

}

Error also needs a causes method, since it mirrors the API of Fail.

Lastly, a nit: I would prefer the name Causes to CauseIter.

also implement `causes()` for `Error` where it will then return the
cause of the `Error` as the first item
@juggle-tux
Copy link
Contributor Author

I changed the name and also added a causes method onto Error.

The causes method on Fail returns now a iterator with it self as the first item to get the most use out of the filter methods on the iterator.

The causes methos on Error returns a iterator with the error cause as the first item since Error does not implement Fail

@tailhook
Copy link

It's interesting how this one interacts with #65. I think it could be made possible to override causes() and walk through the vector of errors.

@withoutboats withoutboats mentioned this pull request Nov 28, 2017
7 tasks
@withoutboats withoutboats merged commit 1494d4b into rust-lang-deprecated:master Nov 28, 2017
@withoutboats
Copy link
Contributor

Thanks for this PR, I've decided to merge it in 🎉

@epage
Copy link

epage commented Nov 28, 2017

Thanks!

@epage
Copy link

epage commented Nov 28, 2017

A big question I think is whether or not this should include self as the first cause. root_cause for example does consider self to be a valid root cause.

What is the rationale for root_cause including self/inner? I tried looking it up in #26 but no reason was given.

It seems surprising without reading the docs carefully. I personally would expect root_cause and causes to not include self/inner.

@withoutboats
Copy link
Contributor

@epage It just seemed like it'd be what you want more often, and semantically more correct. There always is a root cause, if cause returns None then this is the root cause. Or to put it another way, the root cause is defined as the first error that returns None for its cause method, which could very well be self.

If you want the alternative behavior, you can instead do:

let root_cause = err.cause().map(|_| err.root_cause());

@epage
Copy link

epage commented Nov 28, 2017

That makes sense for generally asking the question "what is the root cause" but it feels like it breaks down to me when put in the context of the API. The question is now "What is the root cause for this Error"? An error might not have a root cause. Similarly, when "what are the causes for this Error"? The current error is not one of them.

I know its easy to transform the answers that API gives between different forms. I just want to help ensure that the API is polished, including minimizing surprises. For me, I found the current behavior surprising; maybe thats not the case for everyone. Feel free to move along if you disagree; at least its now recorded in case others ask the question.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants