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 HttpTryFrom<HashMap<String,String>> for HeaderMap #326

Merged
merged 8 commits into from Jul 25, 2019

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Jul 2, 2019

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

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

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 hyperium#325
@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 2, 2019

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

@seanmonstar
Copy link
Member

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

@carllerche
Copy link
Collaborator

👍 on the idea.

@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 3, 2019

@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
Copy link
Contributor Author

lovasoa commented Jul 3, 2019

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

@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 5, 2019

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

@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 11, 2019

Any news?

src/header/map.rs Show resolved Hide resolved
src/header/map.rs Outdated Show resolved Hide resolved
@@ -1720,6 +1724,45 @@ impl<T> FromIterator<(HeaderName, T)> for HeaderMap<T>
}
}

impl<T> Sealed for HeaderMap<T> {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

lovasoa commented Jul 11, 2019

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

@seanmonstar
Copy link
Member

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
Copy link
Contributor Author

lovasoa commented Jul 11, 2019

@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
Copy link
Member

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
Copy link
Contributor Author

lovasoa commented Jul 11, 2019

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

@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 11, 2019

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
Copy link
Member

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
Copy link
Contributor Author

lovasoa commented Jul 12, 2019

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
Copy link
Contributor Author

lovasoa commented Jul 12, 2019

@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
Copy link
Member

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 added a commit to lovasoa/http that referenced this pull request Jul 23, 2019
@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 23, 2019

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

@seanmonstar
Copy link
Member

@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
Copy link
Contributor Author

lovasoa commented Jul 23, 2019

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
Copy link
Contributor Author

lovasoa commented Jul 24, 2019

image

There is no 0.1.x branch...

@seanmonstar
Copy link
Member

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.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much!

@seanmonstar
Copy link
Member

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

@lovasoa
Copy link
Contributor Author

lovasoa commented Jul 25, 2019

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

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.

Implement an easy conversion from HashMap to HeaderMap
3 participants