Add :async-future as an option for async request processing#516
Add :async-future as an option for async request processing#516
Conversation
dakrone
left a comment
dakrone
left a comment
There was a problem hiding this comment.
I left a couple of comments, I see this is a draft though, so I didn't know if it's ready for review.
src/clj_http/client.clj
Outdated
| [req [respond raise]] | ||
| (if (opt req :async) | ||
| [req & [respond raise]] | ||
| {:pre [(not (and (:async req) (:async-future req)))]} |
There was a problem hiding this comment.
These should use (opt req :async) and (opt req :async-future) so they support the optional ? suffix
src/clj_http/client.clj
Outdated
| (opt req :async-future) | ||
| (let [basic-future (org.apache.http.concurrent.BasicFuture. nil)] | ||
| (request (-> req | ||
| (dissoc :async-future) |
There was a problem hiding this comment.
In order to avoid having multiple options I think this should be:
| (dissoc :async-future) | |
| (dissoc :async :async? :async-future :async-future?) |
Sorry for the delay, I've taken your suggestions -- it is ready for review now :) |
|
One thing I really don't like about the approach here is that there is no way to generalize this functionality to For that reason, I'm going to close this PR. |
2 participants
Solves #512.
Notes for Reviwer:
I chose to add a separate option
:async-futureinstead of changing the existing:asyncimplementation to avoid breaking API changes.