http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 83 forks source link

Fix Typed Headers to be consistent #286

Closed Fishrock123 closed 3 years ago

Fishrock123 commented 3 years ago

As discovered in https://github.com/http-rs/http-types/pull/285:

This also uncovers some unpleasantness with the current typed headers - not all of the implement the same methods, one mutates in .value(), some return String or Result<String> instead of HeaderValue, one didn't implement name() either.

Even if we don't make it a trait the consistency should be fixed in 3.0, but we should probably decide on the trait or-not before doing that work.

brightly-salty commented 3 years ago

Do we want a trait like the following?

pub trait TypedHeader {

  fn name(&self) -> Result<HeaderName>;

  fn value(&self) -> Result<HeaderValue>;

  fn to_header() -> Result<(HeaderName, HeaderValue)> {
    (self.name()?, self.value()?)
  }

}
yoshuawuyts commented 3 years ago

I was thinking something closer to:

pub trait Header {
    fn header_name(&self) -> Result<HeaderName>;
    fn header_value(&self) -> Result<HeaderValue>;
}

// NOTE: maybe this should take `Into<HeaderName>` / `Into<HeaderValue>` for compat with existing methods.
// though that may actually complicate things.
impl Header for (HeaderName, HeaderValue) {
    fn header_name(&self) -> Result<HeaderName> {
        Ok(self.0)
    }

    fn header_value(&self) -> Result<HeaderValue> {
        Ok(self.1)
    }
}

There's no need for a separate to_header method since both header_name and header_value take a shared reference to self. Getting both is a matter of calling both methods; and any input trait should be able to take tuples of (HeaderName, HeaderValue) without a problem through a generic impl.

There is one more question on how to actually do the conversions to and from the specific typed header types (e.g. from_headers), but I think those can continue to live on the actual structs. We should take one step at the time here.