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

Display impl for Error eats all the useful information #694

Closed
apoelstra opened this issue Nov 22, 2015 · 9 comments
Closed

Display impl for Error eats all the useful information #694

apoelstra opened this issue Nov 22, 2015 · 9 comments
Labels
E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@apoelstra
Copy link

The Display impl for Error passes through to std::error::Error::description(), which is pretty-much useless. For example, all I/O errors become simply "os error" through this lens.

@seanmonstar
Copy link
Member

Fair point. A nice fix here would be to use the description for hyper errors, but delegate to fmt of errors from other crates.

Example:

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Io(ref e) | Uri(ref e) | Ssl(ref e) | Http2(ref e) | Utf8(ref e) => fmt::Display::fmt(e, f),
            e => f.write_str(e.description())
        }
    }
}

@seanmonstar seanmonstar added the E-easy Effort: easy. A task that would be a great starting point for a new contributor. label Nov 23, 2015
@apoelstra
Copy link
Author

Yeah, I'd be happy with that.

@sfackler
Copy link
Contributor

I tend to go with something like

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        try!(f.write_str(self.description())
        match *self {
            AllThingsWithInnerErrors(ref e) => write!(f, ": {}", e),
            ....
        }
    }
}

@seanmonstar
Copy link
Member

With rust-lang/rust#30312 being merged, at least this should state something more than os error in Rust 1.7, which is an improvement...

@xaviershay
Copy link

I'm getting conflicting messages from this thread. If the problem is that os error isn't useful, that's coming from fmt of a non-hyper IO error. So handing off error formatting of non-hyper errors (such as IO) to fmt wouldn't actually address the initial example. Right?

@apoelstra
Copy link
Author

@xaviershay the problem is that a Display impl is calling description on the std error instead of Display::fmt, so no useful information is propagated, even though it's accessible from the std error.

@seanmonstar
Copy link
Member

@xaviershay it's because the description of io::Error before Rust 1.7 simply says "os error". Instead of printing the description, we could print the full fmt::Debug message of io::Error (or, in 1.7, the description will match the Debug output anyways).

@josephpd3
Copy link

Hey all, I am a little confused on this issue. @seanmonstar, did your PR rust-lang/rust#30312 resolve the matter or am I not quite understanding? Apologies, I'm still getting used to Rust and would like to help if I can.

@seanmonstar
Copy link
Member

@josephpd3 I'm sure that PR improved things. We could make it a little better in hyper by doing as I suggest in the 2nd comment.

DarinM223 added a commit to DarinM223/hyper that referenced this issue Jun 18, 2016
Displays the inner error for Error types with inner errors instead of
just displaying the description.

Closes hyperium#694
seanmonstar pushed a commit that referenced this issue Jun 20, 2016
Displays the inner error for Error types with inner errors instead of
just displaying the description.

Closes #694
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Development

No branches or pull requests

5 participants