Add support for pprint-out slot in eval responses

[cichli]

Adding this directly to nrepl-client.el is obviously not the correct long-term solution, but I'm not sure how best to modify nrepl-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 sets cider-buffer-ns, and also uses nrepl-err-handler, which defaults to cider-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.

(defun cider--ns-response-handler (&optional handler)
  (lambda (response)
    (nrepl-dbind-response response (ns)
      (with-current-buffer buffer
        (when (and ns (not (derived-mode-p 'clojure-mode)))
          (setq cider-buffer-ns ns))))
    (when handler
      (funcall handler response))))

(defun cider-repl--value-response-handler (buffer &optional handler)
  (lambda (response)
    (nrepl-dbind-response response (value)
      (if value
          (cider-repl-emit-result buffer value t)
        (funcall handler response)))))

(defun cider-repl--out-response-handler (buffer &optional handler)
  (lambda (response)
    (nrepl-dbind-response response (out)
      (if out
          (cider-repl-emit-output buffer out)
        (funcall handler response)))))

;; etc.

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?

[bbatsov]

Adding this directly to nrepl-client.el is obviously not the correct long-term solution, but I'm not sure how best to modify nrepl-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 sets cider-buffer-ns, and also uses nrepl-err-handler, which defaults to cider-default-err-handler.

Btw, this function has always had a confusing name, as it creates a handler only suitable for eval requests.

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.

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.

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).

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.

[cichli]

Btw, this function has always had a confusing name, as it creates a handler only suitable for eval requests.

load-file requests too (since by definition they return the same kind of response as eval). It's strangely also used by cider-stdin-handler here even though that op always just returns a constant response of {:status #{:done}}.

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.

[bbatsov]

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.

Sounds good.

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.

Indeed. No way we're going forward with 0.9 until we fix the ClojureScript issues.

[vspinu]

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.

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 buffer argument in handlers and provide a macro that would create a handler with current buffer and current connection stored in its closure. I don't have a good suggestion for the names but here are some sugestive combinations *buffer*/*connection*, nrepl-ibuffer/nrepl-iconnection (i -stands for initial and invocation) or simply nrepl-current-buffer/nrepl-current-connection.

An alternative approach is to handle this problem in the dispatcher and store the callback alongside current buffer and current connection in nrepl-pending-requests. On callback, simply dynamically bind to *buffer* and *connection* just as in previous case.

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 nrepl-pending-requests as we are doing now, but it would be enough to store the callback's symbol. Something like (callback-symbol #ibuffer #iconnection). This is super lightweight and you would not need to bother with cleaning the cache ever, or you can strightforwardly clean it by checking if #iconnection is still alive.

Both approaches are extensible. There will be no difficulty if you ever need to add something else beside ibuffer and iconnection.

[bbatsov]

@vspinu @cichli Please, create a new ticket regarding, so we can continue the discussion in a more focused manner.