shenfeng/http-kit — pull request #6 by ptaoussanis (2013-01-19 14:21:50+00:00)
Title: Experimental: suggested API changes for discussion

Okay, I've posted an alternative `client.clj` for discussion that I think is considerably simpler:
https://github.com/ptaoussanis/http-kit/blob/client-api-fork/src/me/shenfeng/http/client.clj

For comparison, the current `client.clj` is here:
https://github.com/shenfeng/http-kit/blob/client-api/src/me/shenfeng/http/client.clj

The whole thing is ~120 lines, very simple, and very flexible.

Major changes:
**Simplified and refactored some of the utility fns**
You may want to double check that the `query-string` fn still does everything you want it to: I wasn't quite sure exactly what kinds of input yours was designed to support.

**Removed the locking code**
I'm not sure what you were trying to do with this, but I think we should be able to avoid the manual locking with Clojure. I've added a delay to the `get-client-init` fn in case you were worried about possible race conditions when initializing the first client. If that's not the case, then we can remove the delay too.

**Changed the `request` fn args**
Args are now `[url & [method opts-map callback]`.
`method` defaults to `:get`, `callback` defaults to `identity`.

I think this gives a nice balance between being short in the common cases, and completely flexible in all the others:
```clojure
(request "http://www.cnn.com") ; Async GET request
@(request "http://www.cnn.com") ; Sync GET request, returns response
@(request "http://www.cnn.com" :post {:form-params {:name "Bob"}}) ; Sync POST request, returns response
@(request "http://www.cnn.com" :get {}
;; callback:
(fn [{:keys [request status body headers error] :as response}]
(if error (println error) (println body))
) ; Sync GET request, returns value of callback
```

Docstring for the new API:

```clojure
"Issues an async HTTP request and returns a promise object to which the value
of `(callback {:request _ :status _ :headers _ :body _})` or
`(callback {:request _ :error _})` will be delivered.

When unspecified, `callback` is the identity fn.

;; Asynchronous GET request (returns a promise)
(request \"http://www.cnn.com\")

;; Asynchronous GET request with callback
(request \"http://www.cnn.com\" :get {}
(fn [{:keys [request status body headers error] :as resp}]
(if error
(do (println \"Error on\" request) error)
(do (println \"Success on\" request) body))))

;; Synchronous requests
@(request ...) or (deref (request ...) timeout-ms timeout-val)

;; Issue 2 concurrent requests, then wait for results
(let [resp1 (request ...)
resp2 (request ...)]
(println \"resp1's status: \" (:status @resp1))
(println \"resp2's status: \" (:status @resp2)))

Request options: :client, :timeout, :headers, :body, :query-params,
:form-params, :basic-auth, :user-agent"
```

What do you think?

**Changed the `request` fn return value**
Return value is now the result of the callback fn. I think this gives a lot more power than returning just the response. A callback can still choose to return the response if it wants, but it doesn't need to - it has the freedom to decide. I can think of lots of places where this will be useful.

I like your idea of adding a :request key to the callback arguments.

**Changed the `request` error handling**
I removed the `(:error-handler request)`, since I'm not sure what that was for?

I like your idea of separating the :request and :error for the callback argument.

----

Basically, I think the API here is really nice. We can add macros on top of this if you like (personally I don't think it's necessary).

Please take a look at this and let me know what you think? If there's something this does/doesn't do that you don't like, let me know and I'll try fix it up!

Cheers :-)