Add url search params #57
Conversation
anonrig
force-pushed
the
add-url-search-params
branch
4 times, most recently
from
cb742ba to
4816774
Compare
anonrig
force-pushed
the
add-url-search-params
branch
from
4816774 to
ac2efd0
Compare
There was a problem hiding this comment.
nitpicking as requested per https://bsky.app/profile/yagiznizipli.com/post/3lcb3fi6v4s26
There was a problem hiding this comment.
I agree with all the points @untitaker has raised.
As suggested, I would also auto-generate the C(++)?-ffi bindings.
Then, I would make all mutating methods &mut self.
For strings, it might be worth converting those to either &CStr or &str at the FFI boundary, depending on what bindgen it doing.
anonrig
force-pushed
the
add-url-search-params
branch
from
b9811ac to
2ab1c7f
Compare
|
hey @untitaker @Swatinem @mitsuhiko, thank you for all the reviews! I've tried to address them all, and learned a lot along the way. Would you mind taking a one last take? Please nit-pick / be super-picky, and don't lower your personal barrier (just for the sake of positivity). |
There was a problem hiding this comment.
lgtm, though some more polish would be needed:
- which length are we talking about? length in serialized bytes? number of unique keys? number of total values?
- it should be made clearer in docs, and maybe also in the types how exactly the multi-map works.
So this means clarifying and maybe also giving an example for accessors that yield the first value, vs those that yield all the values. For example, does entries() give you just the first value, or does it rather give you the same key multiple times with each of its values?
Same for the remove and contains method, please clarify how the Option-al value relates to the multi-map.
looking up MultiMap on crates/docs.rs gave me this crate which has for example iter that yields keys and their first value, plus flat_iter which I believe would duplicate keys for each value, though I believe the docs are also wrong/unclear there: https://docs.rs/multimap/latest/multimap/struct.MultiMap.html#method.flat_iter
Another question related rather to URLs would be whether a value-less key is even valid? Like ?a&b&c?
| impl<Input> Extend<(Input, Input)> for UrlSearchParams | ||
| where | ||
| Input: AsRef<str>, |
There was a problem hiding this comment.
You could use two different generics here, that way you could use a &str, Cow<'_, str> or whatever other combination of key/value types, instead of being forced into using the same type for key/value.
There was a problem hiding this comment.
What's the syntax for supporting multiple generics for the same variable?
| length: usize, | ||
| ) -> *mut ada_url_search_params; | ||
| pub fn ada_free_search_params(search_params: *mut ada_url_search_params); | ||
| pub fn ada_search_params_size(search_params: *mut ada_url_search_params) -> usize; |
There was a problem hiding this comment.
So, on the -> usize bit here, this isn't wrong but it is something that's like, being debated. That is, if usize is size_t or not. Today, it's often used that way, but this can cause issues on platforms like CHERI. However, there's tons of code written like this, so anything that happens will have to deal with all of that... so I'm not saying you shouldn't do this, just wanted to clue you in on that as a general thing.
There was a problem hiding this comment.
I remember reading about this. Nice catch. What's the "best" solution to use for usize?
Some traits are missing. Since I'm not using Rust daily, I don't know which traits are beneficial to be implemented into the new structs.
Please review and give recommendations around the missing structs and the API surface.