search

http-rs / http-types

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

feat: impl ToHeader for all typed headers #285

Open Fishrock123 opened 3 years ago

Fishrock123 commented 3 years ago

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.

jbr commented 3 years ago

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 commented 3 years ago

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

yoshuawuyts commented 3 years ago

I believe we talked about this before, but I think a Header type as outlined in https://github.com/http-rs/http-types/issues/286#issuecomment-750890828 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).