Closed martinklepsch closed 4 years ago
@martinklepsch looks good, thanks for doing this!
About execute-ctx
returning :response
. It looks like only await-result
and deliver-result
care about the :response
. Could they maybe be passed a result-getter function instead that is identity
for execute-ctx
and :response
for execute
? It's less DRY but avoids leaking :response
into the execute-ctx
API.
Here it is sketched out (untested)
#?(:clj
(defn- await-result [ctx]
(if (a/async? ctx)
(recur (a/await ctx))
(if-let [error (:error ctx)]
(throw error)
ctx)))) ; <---
(defn- deliver-result [ctx get-result] ; <---
(if (a/async? ctx)
(a/continue ctx deliver-result)
(let [error (:error ctx)
result (or error (get-result ctx)) ; <---
callback (if error :on-error :on-complete)
f (get ctx callback identity)]
(f result))))
(defn execute-ctx
{:arglists '([interceptors initial-context]
[interceptors initial-context on-complete on-error])}
([interceptors initial-context on-complete on-error]
(execute-ctx interceptors initial-context on-complete on-error identity)) ; <---
([interceptors initial-context on-complete on-error get-result]
(if-let [queue (q/into-queue interceptors)]
(-> (context initial-context queue on-complete on-error)
(enter)
(leave)
(deliver-result get-result)) ; <---
;; It is always necessary to call on-complete or the computation would not
;; keep going.
(on-complete nil))
nil)
#?(:clj
([interceptors initial-context]
(when-let [queue (q/into-queue interceptors)]
(-> (context initial-context queue)
(enter)
(leave)
(await-result))))))
(defn execute
([interceptors request on-complete on-error]
(execute-ctx interceptors {:request request} on-complete on-error :response)) ; <---
#?(:clj
([interceptors request]
(some-> (execute-ctx interceptors {:request request})
:response)))) ; <---
I ended up just adding execute-ctx
which duplicates most of the code of execute
.
@den1k I've added you as a collaborator to my fork, please feel free to help with updating/expanding the test suite and testing in general. I'd love to get this into a place where it could be considered for inclusion in Sieppari itself but also only have limited time to hack on this...
@martinklepsch I'm in the same boat time-wise at least until the end of this week. However, it looks like you already made changes to src + tests. Is this ready for review?
closing in favor of #31
This tries to address some of the requests (#15 & #28) for a more context focused API (i.e. without the request/response concepts).
execute-ctx
still returns only the:response
value but that's probably something we can figure out.The main goal here is that we can move forward with a Context-based API while maintaining the previous request/response style for backwards compatibility.
cc @dspiteself @jjttjj @den1k