-
-
Notifications
You must be signed in to change notification settings - Fork 649
Add support for pprint-out
slot in eval responses
#1098
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
Conversation
Btw, this function has always had a confusing name, as it creates a handler only suitable for
Sounds reasonable to me. I never liked the current approach. It's legacy from the very first nrepl.el release and I always planned to improve this at some point.
You can also build a few "standard" handlers, relying on the composition approach. Alternatively we could have a macro that builds the handler taking a list of responses and callbacks for them. Similar to what we have now, but a bit more succinct and flexible. I definitely think that passing functions as arguments to this function (as we do now) is a terrible API. |
I'll proceed with the function composition changes on another branch, so you can merge this beforehand if you wish to get it in before 0.9.0. Although I think the CLJS completion issue and #1088 are both pretty big blockers for the release, and I'm not sure how long the latter will take to resolve. |
Sounds good.
Indeed. No way we're going forward with 0.9 until we fix the ClojureScript issues. |
This is definitely the way to go, but there is a trick to it. I recall @bbatsov and I have discussed this before and I even had a proposal for a handy macro. Unfortunately I am not able to dig that conversation anymore, but here is the idea. The core issue is that besides invocation buffer (current buffer during request) each handler must also accept the invocation connection. The motivation is obvious, just like with buffers, current connection can change before the response arrived. One idea was to drop An alternative approach is to handle this problem in the dispatcher and store the callback alongside current buffer and current connection in The first lexical approach is cleaner, but the dynamic approach has two advantages. First is that you don't need a constructor macro for the handler (the user can simply provide a function and rely on dynamic bindings). Second is the storage consideration, you need not store full callback in Both approaches are extensible. There will be no difficulty if you ever need to add something else beside ibuffer and iconnection. |
Add support for `pprint-out` slot in eval responses
Adding this directly to
nrepl-client.el
is obviously not the correct long-term solution, but I'm not sure how best to modifynrepl-make-response-handler
to make it open to this kind of extension. It already has a couple of other CIDER-specific things that we should pull out - it setscider-buffer-ns
, and also usesnrepl-err-handler
, which defaults tocider-default-err-handler
.Perhaps we should instead create response handlers by composing higher order functions that take any relevant arguments, and optionally the next handler to be called, e.g.
A caller making an nREPL request will then simply construct their response handler using function composition:
(->> (cider-repl--out-response-handler buffer) (cider-repl--value-response-handler buffer) (cider--ns-response-handler) ...)
This is, IMO, a nicer and more flexible API than having one monolithic
nrepl-make-response-handler
that takes six arguments, five of them being functions with slightly differing arities.The trade-off being that it's a bit more verbose, and now the caller needs to concern itself with how the dispatching of handlers occurs (although we could always leave
nrepl-make-response-handler
there for simpler cases where the defaults work fine).Thoughts?