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

Use MultiMap instead of HashMap for the parsed query params #57

Open
havarnov opened this issue Dec 2, 2015 · 9 comments
Open

Use MultiMap instead of HashMap for the parsed query params #57

havarnov opened this issue Dec 2, 2015 · 9 comments

Comments

@havarnov
Copy link

havarnov commented Dec 2, 2015

Use multimap for the parsed query params instead of HashMap<String, Vec<String>>.

Mulitmap is basically a thin wrapper around HashMap<T, Vec<T>>, but with (imo) a bit better api for using the values. Especially for the common use case of only having one value per key. E.g.:

let params: HashMap<String, Vec<String>> = HashMap::new();
let val = params["key"][0];
for (key, vec_of_val) in params {...}

let params: MultiMap<String, String> = MultiMap::new();
let val = params["key"];
for (key, val) in params.iter() {...}
for (key, vec_of_val) in params.iter_all() {...}
@reem
Copy link
Member

reem commented Dec 3, 2015

If multimap can provide the flexibility of the full HashMap API (I am mostly concerned about iterators and the entry API in particular) then I am open to this.

@untitaker
Copy link
Member

I think the additional methods provided by multimap could be provided by a trait that is impl'd for HashMap<K, Vec<V>>, not a wrapper. This would provide full backwards compat, urlencoded wouldn't have to depend on multimap, and no typesig would have to be changed.

@havarnov
Copy link
Author

havarnov commented Dec 4, 2015

I don't see how you could implement for example .get(key) which returns Some(val)|None and not the whole vector, but maybe I'm missing something. And that is IMO the biggest win because the most common case is only having one value per key.

The "custom" iterators could probably be implemented as a trait though.

@untitaker
Copy link
Member

You'd have to rename it to get_only or use it directly from the trait to avoid nameclashes. That's part of backwards compat.

@untitaker
Copy link
Member

@havarnov any update on this? Would you consider changing your multimap crate in such a way, or should I write my own version?

@untitaker
Copy link
Member

Also missing voice of @reem

@reem
Copy link
Member

reem commented Dec 16, 2015

My thoughts haven't really changed, if the API is as good as HashMap I am leaning towards it.

@havarnov
Copy link
Author

Havn't had time to look at think about this. I don't want to make MultiMap less convinent because that's the whole point. Hmm, let med look at it through the weekend.

@havarnov
Copy link
Author

Sorry for the radio silence, haven't had time to look into this before now. I've made a version which uses MultiMap instead of HashMap: https://github.com/havarnov/urlencoded/tree/feat/multimap. Maybe it's easier to compare the options now..

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

No branches or pull requests

3 participants