Implement HttpTryFrom<HashMap<String,String>> for HeaderMap

[lovasoa]

This allows an easy conversion from HashMap<String,String>
to HeaderMap.

Doing this conversion used to be unnecessarily verbose
because of the required mapping and error type conversions.

The implementation is generic:

    impl<COLLECTION, K, V> HttpTryFrom<COLLECTION> for HeaderMap<HeaderValue>
    where
        COLLECTION: IntoIterator<Item=(K, V)>,
        K: AsRef<str>,
        V: AsRef<str>

so it also works for HashMap<HeaderName, String> and for vectors.

Fixes #325

[lovasoa]

The broken CI is not due to the changes introduced in this PR, but to #327.

[seanmonstar]

Hm, what if instead of K and V being AsRef, they were HeaderName: HttpTryFrom<K> and HeaderValue: HttpTryFrom<V>?

[carllerche]

👍 on the idea.

[lovasoa]

@seanmonstar OK, but I will have to add some HttpTryFrom implementations to HeaderName and HeaderValue, then, if we want it to still work with HashMap<String,String>.

[lovasoa]

@seanmonstar : I made the change you suggested. It required adding an impl HttpTryFrom<&String> for HeaderName and HeaderValue.

[lovasoa]

@seanmonstar Is it ok like that, or is there still something you would like me to change ?

[lovasoa]

Any news?

[seanmonstar]

There should probably be an identity impl, impl<T> HttpTryFrom<HeaderMap<T>> for HeaderMap<T>. Would that conflict with the generic one below?

[lovasoa]

Yes, HeaderMap<T> is itself IntoIterator<Item=(HttpTryFrom<HeaderName>, HttpTryFrom<HeaderValue>)>, so HeaderMap::try_from(header_map) works as expected.

[seanmonstar]

Usually one would expect TryFrom<Me> for Me to be basically free, but because that conflicts, it would be surprising that it essentially clones a new HeaderMap....

[lovasoa]

This is indeed sub-optimal, and can fixed later when rust adds support for specialization.
But until then, I think the usability benefits far outweigh this downside. The standard library itself has the same issue in several places, where an implementation is slower than it could be in order to be more generic.

[lovasoa]

@seanmonstar I implemented your the changes you suggested. Hope this can be merged soon.

[seanmonstar]

It's worth noting that the current conversion can be done fairly ergonomically already:

let mut map = HashMap::new();
map.insert("hello", "world");
map.insert("and", "good day");
    
let headers = map
    .into_iter()
    .map(|(name, value)| {
        Ok((
            name.parse()?,
            value.parse()?,
        ))
    })
    .collect::<Result<HeaderMap, http::Error>>();

[lovasoa]

@seanmonstar The main advantage of implementing HttpTryFrom is not only to remove all these lines of boilerplate, but also to allow libraries to provide functions that accept a trait instead of having the users manually converting to custom types all the time. Also, shouldn't it have been discussed earlier ?

[seanmonstar]

shouldn't it have been discussed earlier ?

True, I'm very sorry I didn't realize how to make the original request shorter. I didn't think there would be a conflict with a generic HttpTryFrom implementation.

I recognize what the goal is, I'm just pointing out what exists already. I believe that the identity implementation isn't "free" would be surprising to everyone.

[lovasoa]

@seanmonstar I answered above. I see what you mean, but I don't think this should block merging this. Do you ?

[lovasoa]

I prefer the generic option, but if you think the slight HeaderMap to HeaderMap conversion overhead is a real problem people would encounter, we can switch to having a separate implementation for a few standard library containers (maybe only Vec and HashMap), and that wouldn't conflict with HttpTryFrom<HeaderMap> for HeaderMap.

[seanmonstar]

A reason I think it'd be surprising is, for example, if some Builder were to change their headers method to one of HeaderMap: HttpTryFrom<T>, a user who already had a HeaderMap would end up paying twice.

For the completely generic situation, I think the code I pasted a few comments ago is ok. If wanting to add HttpTryFrom anyways, it's probably best to just provide it for concrete std collections for now, like HashMap. Providing one for Vec<(K, V)> seems fairly specific, is it that common?

[lovasoa]

Vec<(K,V)> would allow using the standard vec! macro, which allows readable one-line definitions of headers. I can omit it if you want, but I see no reason to.

[lovasoa]

@seanmonstar I added a marker trait to limit the generic implementation to just a few standard library types, so that the generic implementation does not apply to HeaderMap itself. Tell me what you think ?

[seanmonstar]

I appreciate the goal, but I think there's value in staying conservative, because we can't really make breaking changes, since http is meant to be an interoperable crate for the whole ecosystem.

We could start with just HashMap, and then adding others in the future is not a breaking change. How about we start there?

[lovasoa]

@seamonstar I removed the implementation for non-HashMap types. I hope this can be merged now.

[seanmonstar]

@lovasoa Thank you! I see this is targeting master, but I assume you want to use this before 0.2 releases? If so, if you can change the target to hyperium:0.1.x, I can merge to the right branch.

Would you be able to remove the marker trait for now, since we'll only have the HashMap implementation to start, so as to not have more in the public API? Or I can do so as well during the merge.

[lovasoa]

Ok, I'll change the target of the PR. I think the marker trait is still needed to implement the conversion from both HashMap and &HashMap. Or is there another way?

[lovasoa]

There is no 0.1.x branch...

[seanmonstar]

Woops, I'm so sorry, I got confused. Most of the other repos I'm working in have converted master to the newer version, but this one still has master as 0.1. I'll merge this in first, and then update to be more consistent.

[seanmonstar]

Thanks so much!

[seanmonstar]

Er, it seems there was no into_iter for &HashMap in Rust 1.20?

[lovasoa]

@seanmonstar I fixed rust 1.20 compatibility. I think this is ready for merging.