Augment RegisterResponderWithQuery function #61
Conversation
There was a problem hiding this comment.
Fist off, cool you're working on this. Maintainers haven't been able to keep up, so I'd recommend a fork and just merge your changes in there.
A few general pieces of feedback: https://github.com/jarcoal/httpmock/pull/61/files#diff-0fecf8d8a1c3b52ef05e62d6a1bf6effR449 (and other methods, I think) should arguably all get updated to take a properly structured net.Values object. The API was wrong to begin with and I would be more agressive about cleaning it up, but release with a bump on the major version.
I also think having RegisterResponderWithQueryValues and RegisterResponderWithQuery is a confusing API. Like, which do I use? I would just make the more aggressive change of compacting them to have the behavior of the RegisterResponderWithQueryValues with the name RegisterResponderWithQuery. If you don't want to do that, RegisterResponderWithQuery should at least call RegisterResponderWithQueryValues under the hood.
|
@Zanadar I agree that having both Of course we can bump the major version, but I prefer not. IMO having a new branch will add more confusion than having 2 similar but well documented methods, even if the API is not as clean as it could be in this case. Note that if you have a better name for By the way, |
|
@Zanadar I just merged |
maxatome
changed the title
|
Unit tests OK. |
query type can now be: - url.Values - map[string]string - string, a query string like "a=12&a=13&b=z&c" (see net/url.ParseQuery function) Summary: - allow to pass several values for one query parameter; - fix the case when RegisterResponder register an url with unsorted query params: RoundTrip() now checks with url & untouched query params before falling back to sorting query params; - code simplified to rely on net/url.Values. Signed-off-by: Maxime Soulé <btik-git@scoubidou.com>
query params: RoundTrip() now checks with url & untouched query
params before falling back to sorting query params;
Could replace #55 & #57 what do you think of this, @Zanadar and @kardolus?
RegisterResponderWithQueryValuescan be renamed if you have a better idea.