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

feat: impl ToHeader for all typed headers #285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Nov 25, 2020

This adds a to_header(self) -> Result<(HeaderName, HeaderValue)> for every Typed header, as well as anything that can TryInto a (HeaderName, HeaderValue) pair.

This should make typed headers nice to use with builds such as Surf's RequestBuilder, where .header(name, value) could be changed to .header(ToHeader) and also still work with a raw tuple.


I'm not 100% happy with this though - notably to have atrait like this either we need it to be consuming (i.e. consume a typed header) or it has to clone if a tuple is passed.

An alternative would be to have every header impl Into<(HeaderName, HeaderValue)>...


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.

I think we should probably consider rolling that all into a trait.

This adds a `to_header(self) -> Result<(HeaderName, HeaderValue)>` for every Typed header, as well as anything that can `TryInto` a `(HeaderName, HeaderValue)` pair.

This should make typed headers nice to use with builds such as Surf's `RequestBuilder`, where `.header(name, value)` could be changed to `.header(ToHeader)` and also still work with a raw tuple.
@jbr
Copy link
Member

jbr commented Dec 8, 2020

I haven't kept up with the typed header unification trait, but is it ever the case that a typed header would need to add multiple headers?

@Fishrock123
Copy link
Member Author

None of the existing ones do, it seems, although some parse multiple headers if there are multiple, I think.

@yoshuawuyts
Copy link
Member

I believe we talked about this before, but I think a Header type as outlined in #286 (comment) might be preferable. This allows us to concretely talk about a Header which contains a HeaderValue and HeaderName, rather than it remaining an un-named concept (ToHeader converts to a tuple, which isn't a header per se, but serves a same role. I propose we actually create a concrete "header" concept).

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.

3 participants