puppetlabs / clj-http-client

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

Add a general request function to the client protocol. #46

Closed adreyer closed 8 years ago

adreyer commented 8 years ago

There are a few places where we map request method keywords to http-client function. The http client then maps them back to keywords which is redundant. This adds a general request function to the protocol that accepts a keyword method.

All the other methods could be tweaked to use request. I'm not sure if that's worth it.

rlinehan commented 8 years ago

@adreyer travis failures on this seem to be legitimate - I get the same when I run locally

ERROR in (persistent-async-client-test) (AFn.java:429)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (2) passed to: async/eval12700/request-with-client--12705
                                                                  ...                        
puppetlabs.http.client.async/eval12724/create-client/fn/reify/request                         async.clj:  466
...
adreyer commented 8 years ago

@fpringvaldsen @rlinehan this looks fixed was there anything else todo?

rlinehan commented 8 years ago

I think it would be good to add some tests - probably to https://github.com/puppetlabs/clj-http-client/blob/master/test/puppetlabs/http/client/async_plaintext_test.clj#L74 and somewhere in https://github.com/puppetlabs/clj-http-client/blob/master/test/puppetlabs/http/client/sync_plaintext_test.clj.

Otherwsie I think I'm :+1:.

adreyer commented 8 years ago

After using this I moved the new function to "make-request".

adreyer commented 8 years ago

I will soon be blocked on this @rlinehan @fpringvaldsen do either of you have time to review merge?

cprice404 commented 8 years ago

@camlow325 I think @rlinehan is on PTO today so maybe you could review this since you're already in this repo

MSLilah commented 8 years ago

The new tests look good and the tests are passing for me locally, so I'm going to go ahead and merge this.