Skip to content

Fix Typed Headers to be consistent #286

Closed
@Fishrock123

Description

@Fishrock123

As discovered in #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.

Activity

added this to the 3.0 milestone on Nov 25, 2020
brightly-salty

brightly-salty commented on Dec 19, 2020

@brightly-salty
Contributor

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

yoshuawuyts commented on Dec 24, 2020

@yoshuawuyts
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    semver-majorThis change requires a semver major change

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      Participants

      @Fishrock123@yoshuawuyts@brightly-salty

      Issue actions

        Fix Typed Headers to be consistent · Issue #286 · http-rs/http-types