puppetlabs / clj-http-client

HTTP client library wrapping Apache HttpAsyncClient
Apache License 2.0
15 stars 30 forks source link

(TK-29) Add support for query parameters #10

Closed MSLilah closed 10 years ago

MSLilah commented 10 years ago

This PR adds support for query parameters to both the Java and clojure clients.

With the Java client, setting Query Parameters works the same way it did before the port to AsyncHttpClient.

With the clojure client, query parameters are set with a new :query-params option in the options map passed to a request. Query parameters can be set either in the URL or with this new option. However, if the query parameters are set with this new option AND in the URL, all parameters set directly in the URL will be removed, and only the ones specified in the options map will be sent as parameters.

KevinCorcoran commented 10 years ago

:+1:

MSLilah commented 10 years ago

@KevinCorcoran I've addressed your comment

camlow325 commented 10 years ago

Maybe it would be too late to do this now and maintain compatibility but I think it would be better for the RequestOptions Java class to require the caller to specify an instance of a java.net.URIclass -- in calls to the constructor and on the getUrl() and setUrl() methods. That way, we wouldn't need to have a separate set of accessors for query params and all of the ambiguity around how query parameters would be handled when included in both the string argument to the constructor and the accessor methods would go away.

camlow325 commented 10 years ago

:+1: overall although I would like to get some feedback on a few points:

KevinCorcoran commented 10 years ago

@camlow325 - I think that's a good idea. I was also surprised when I looked at RequestOptions.url and found it to be a String.

Also, I don't think we need to worry about backwards-compatibility.

MSLilah commented 10 years ago

@camlow325 @KevinCorcoran I believe your comments have been addressed.

camlow325 commented 10 years ago

:+1: with a few trailing comments:

MSLilah commented 10 years ago

@camlow325 I've addressed your comments.

camlow325 commented 10 years ago

:+1: