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

Implement the Header trait #315

Merged
merged 4 commits into from
Feb 12, 2021
Merged

Implement the Header trait #315

merged 4 commits into from
Feb 12, 2021

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Dec 27, 2020

Closes #286

This PR implements the Header trait, creating a unified interface for the convention we had of TypedHeader::{value, name, apply}. This instead enables an interface of:

pub trait Header {
    fn header_name(&self) -> HeaderName;
    fn header_value(&self) -> HeaderValue;
    fn apply_header<H: AsMut<Headers>>(&self, mut headers: H);
}

Because header_name and header_value have to be implemented, apply_header has a default implementation. This should make it easier to keep up to date.

Breaking changes

This patch doesn't break anything, but it does set us up for the 3.0.0 release. When we're ready to implement the 3.0.0 changes we should remove the name, value and apply methods in favor of keeping the trait only. This patch also explicitly chose to keep the value method implementation where it is in order to not create a massive merge conflict with #313.

Bug fixes

This patch also fixes two bugs: a wrong header name in TE and in TransferEncoding. These have been adjusted and now use the right header name.

Prior work

This patch serves as an alternative to #285, implementing the design I referenced in #285 (comment).

Future directions

We should consider taking a look at the headers submodule in general. A few changes I think would be worthwhile:

  • Move the header name constants from the crate root to be associated constants on HeaderName instead.
  • Rename Headers to HeaderMap, matching the hyperium/hyper naming convention
  • Consider changing the return type of ToHeaderValues from an associated iterator to be an instance of HeaderValues instead. The trait precedes the concrete type, and having a trait and a type with similar names be completely unrelated feels awkward.
  • Do a pass on the typed headers to ensure the right AsRef and AsMut implementations are provided.
  • Do a pass on the typed headers to clean up the examples, making them practical.
  • Do a pass on the typed headers to link each header to their respective MDN documentation.
  • Implement per-method examples on the typed headers.
  • Create http_types::headers crate-level docs with an explainer of the different types in the crate.

@Fishrock123
Copy link
Member

Fishrock123 commented Jan 20, 2021

Can (&str, &str) implement this Header trait?

I ask because I think it's still the best way to solve the Surf .header(header) issue.

@yoshuawuyts
Copy link
Member Author

@Fishrock123 good question; will have to try. I suspect there may actually be limits on what we can do because of how we're using generics.

@yoshuawuyts
Copy link
Member Author

Oh woah, the impl actually worked! Added a test for it too:

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn header_from_strings() {
        let strings = ("Content-Length", "12");
        assert_eq!(strings.header_name(), "Content-Length");
        assert_eq!(strings.header_value(), "12");
    }
}

@yoshuawuyts
Copy link
Member Author

It's been a while. Assuming this is good -- going to merge it.

@yoshuawuyts yoshuawuyts merged commit 507f15a into main Feb 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the header-trait branch February 12, 2021 11:52
@Fishrock123
Copy link
Member

Sorry. I think this should probably be able to deal with borrows if possible, see #336/#335

@yoshuawuyts yoshuawuyts mentioned this pull request Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Typed Headers to be consistent
2 participants