Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bypass fn #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

bypass fn #24

wants to merge 2 commits into from

Conversation

england
Copy link

@england england commented Feb 8, 2018

Hi, this is a bypass function implementation. My use case is open clj-http request results in browser for manual verification.

@gfredericks
Copy link
Owner

This is a function that gets called on the response, regardless of whether you're recording or playing back?

@england
Copy link
Author

england commented Feb 8, 2018

Yes

@gfredericks
Copy link
Owner

hmm; I wonder if this can be more general

e.g., an option like:

:middleware
(fn [vcr-clj-fn]
  (fn [& args]
    ;; can do something beforehand
    (let [res (apply vcr-clj-fn args)]
      ;; or afterwards with the result
      res)))

@england
Copy link
Author

england commented Feb 8, 2018

Mm, yeah, it's more low-level, but it will cover my needs

@gfredericks
Copy link
Owner

okay, I think I could accept a change like that

@england
Copy link
Author

england commented Feb 9, 2018

What is the pros of this approach in comparison with :middleware (fn [vcr-clj-fn & args] ...) ?

@gfredericks
Copy link
Owner

they're equivalent; the word "middleware" makes me think of ring middleware, which uses the signature I suggested. In contrast, I know robert.hooke uses the signature you mentioned. I don't strongly prefer either.

@gfredericks
Copy link
Owner

I will get to this soon. I've been sick for a week and need to catch up on things.

@@ -33,7 +35,7 @@
(if-not (and *recording?* (apply recordable? args*))
(apply orig-fn args*)
(let [res (binding [*recording?* false]
(return-transformer (apply orig-fn args*)))
(return-transformer (apply (middleware orig-fn) args*)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to preview a response body in a web browser, I'm thinking you'll need the middleware to be applied after the return-transformer so that the body has already been made re-readable, does that sound right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having the middleware see the transformed return value should also make it more consistent with the way it's called during playback.

:or {arg-transformer vector
arg-key-fn vector
recordable? (constantly true)
middleware (fn [f]
(fn [& args] (apply f args)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be simplified to identity

(let [k (apply arg-key-fn args*)
res (:return (the-playbacker the-var-name k))]
(apply (middleware (fn [& args] res)) args*))
(apply (middleware orig) args*))))]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I would expect the middleware to be called in the non-recordable case; what do you think?

(apply orig args*))))]]
(let [k (apply arg-key-fn args*)
res (:return (the-playbacker the-var-name k))]
(apply (middleware (fn [& args] res)) args*))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the middleware could be applied to the real playback function rather than an artificial function via something like

(apply (middleware (fn [& args] (:return (the-playbacker the-var-name (apply arg-key-fn args))))))

(args-test args)
(let [res (apply vcr-clj-fn args)]
(res-test res)
res)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you either move these defns to the top level or transform them into a let or letfn? I don't think nested defs are idiomatic in clojure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants