Consider moving to only one error type about http HOT 12 CLOSED

I've never used error chain...

Would it work if there was an http::Error type and conversions from all other errors into http::Error?

from http.

Ah I think error chain itself isn't so much of a thing to worry about here but rather the interconversion of so many error types. The error chain pattern is one where it's nice to only have one error per lib you're workign with (rather than N inside each of the M libs), but in general it's nicer to talk about just one kind of error vs many.

Would it work if there was an http::Error type and conversions from all other errors into http::Error?

I was hoping so! I initially though we could do this but unfortunately it doesn't work. What we're looking at is functions that are like:

fn foo() -> MyResult<()> {
    some_http_function()?;
}

So the ? operator needs a conversion from all errors in the HTTP crate to MyError (or w/e the error is in MyResult). The problem is that it needs a direct conversion, so you'd have to implement it once per HTTP error.

Another reason we might want this is that there's no ergonomic method of writing a function that calls multiple HTTP apis but only return one kind of error, right now you'd be forced to always define your own!

from http.

I'm Ok w/ whatever @seanmonstar / @withoutboats agree to.

from http.

In the conversion errors, I think it is not helpful to distinguish source type in the errors. That is, I think these sets of errors should be merged into a single zero sized type:

• status::FromU16Error and status::FromStrError
• header::name::FromBytesError and header::name::FromStrError

These errors should also all be renamed so that they just indicate that the value of this type was invalid - similar to InvalidValueError they should be InvalidStatusError, InvalidNameError, etc, instead of saying they are a From{X}Error.

It could be a convenience to also have a unified error type which indicates all the ways constructing an HTTP message could fail, to be used either by the request/response builders (depending on how they shake out), or just to make it easier for downstream users to coallesce these errors together.

from http.

I suppose a lot of this issue can be boiled down to the builder PR, for example, what does this look like?

fn foo() -> Result<(), ???> {
    let request = Request::builder()
        .header("Foo", "Bar") /* ? */
        .method("POST") /* ? */
        .uri("https://www.rust-lang.org") /* ? */
        .body(); /* ? */

    // use request
}

How many ? is necessary to get that working, and what error do we fill in to get it working? I've been thinking we want one ? operator and a "by default" error to fill in there, but we could move the slider elsewhere perhaps!

from http.

Right, my thinking was that if each of those methods return Results, we could either:

1. Have them return a unified error type.
2. Have a unified error type exist, so they don't have to, but foo could return it and they could all be ?'d because of the From impl.

The second seems slightly more "pure" but also less obvious how you can use it ergonomically.

from http.

Have a unified error type exist, so they don't have to, but foo could return it and they could all be ?'d because of the From impl.

The downside here is that it won't jump 2 Intos for you. So, if each method returns a separate error type, then either this doesn't work, or you gotta implement From a whole bunch of times:

fn do_a_thing() -> Result<(), MyError> {
    let req = Request::builder()
        .method("GET")?
        .uri("https://www.rust-lang.org")?
        .build();
}

from http.

Yeah that's what I was thinking as well. The downside of (2) you listed though is that let's say you have:

fn foo() -> Result<(), MyCustomError> {
    let request = Request::builder()
        // ...
        .body(());

    // large block of code returning `MyCustomError` everywhere
}

So here you're using the http crate in the context of a larger application, which means that your function now returns your application's error instead of http crate's "blanket error". In this case though you're required to do:

impl From<http::Error> for MyCustomError {}
impl From<http::InvalidUri> for MyCustomError {}
impl From<http::InvalidStatus> for MyCustomError {}
impl From<http::InvalidHeaderName> for MyCustomError {}
impl From<http::InvalidHeaderValue> for MyCustomError {}
// ...

That's what initially led me down the road of one blanket error everywhere. I figured that the ergonomics of converting HTTP errors into your application would come up quite frequently, whereas inspecting the error cause would come up quite rarely, so I figured we could optimize the other way for now. Having only one error type though is certainly far less flexible!

from http.

That's a good point. I'd be happy using 1 error type in the request/response builder, and if users want to avoid the "overhead" of that type they can use the methods like HeaderValue::from_bytes etc.

from http.

Er this comment maybe should have been placed here instead

from http.

Do you think though that rationalizing this with the rest of the library would make sense? For method_from to return a http::Error but Method::try_from_slice("POST") would return a http::InvalidMethod error?

Its definitely an unfortunate outcome, but it seems like our options are (in a sort of spectrum):

• Only have a unified error type, no individual types.
• Have the builders return a unified error type, but the other methods return individual error types.
• Have a unified error type, but everything returns individual types and you have to perform the cast.
• Have no unified error type, only individual types.

All of these have downsides. I lean toward the second one but I could probably be convinced of the first if you think we should do that. The downside of moving away from individual types I see is that with ZSTs I'm pretty sure you get the NPO today if you use things like HeaderValue::from_bytes.

from http.

An excellent way to put it!

It seems like we can "safely" eliminate the last one at least. I'd be a little less worried about the ZST-ness of the errors in the sense of that they're all in theory going to grow more payload information over time. (in the sense that we probably wouldn't want to guarantee ZST-ness)

I've been wondering if we could have a unified error with tons of accessors or something like that. In general it seems like 90% of users don't need the benefit of fine-grained errors, so if we could continue to cater to the 10% with perhaps a little less 'static guarantees' it may be a win-win, but unsure.

from http.