Fix toggle connections

[dpsutton]

• The commits are consistent with our [contribution guidelines][1]
• You've added tests (if possible) to cover your change(s)
• All tests are passing (make test)
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)
• You've updated the refcard (if you made changes to the commands listed there)

Thanks!
issue 1913

This introduces a new "type" of cider repl, as returned from cider-connection-type-for-buffer. In addition to clj and cljs, we have cljc. This type will use the first connection in cider-connections. So we can easily reorder this list to change the default connection.

Previously, since connections were inferred from what was available and what mode you were coming from, this toggle function would create a buffer local var for cider-connections and remove those connections. The problem was that you could not toggle them again as your list ran out of connections. This keeps all connections around and makes sure that cider-map-connections and others work well with multiple connections.

[bbatsov]

This introduces a new "type" of cider repl, as returned from cider-connection-type-for-buffer. In addition to clj and cljs, we have cljc. This type will use the first connection in cider-connections. So we can easily reorder this list to change the default connection.

I don't understand this. Our idea for cljc files was to dispatch over both connections when available (clj and cljs).

Previously, since connections were inferred from what was available and what mode you were coming from, this toggle function would create a buffer local var for cider-connections and remove those connections. The problem was that you could not toggle them again as your list ran out of connections. This keeps all connections around and makes sure that cider-map-connections and others work well with multiple connections.

Hmm, I'll have to check. This function was intented to be used only in scratch buffers and toggle between their Clojure and ClojureScript connection. I wrote it in a hurry, I might have overlooked something.

[dpsutton]

@bbatsov this actually wouldn't change that mechanism. Anytime something goes through (cider-map-connections ... :both) it will transport across up to two relevant connections. And it does this by saying (cider-other-connection repl-buffer). So your notion is not impacted at all.

One problem is that this seems to be very haphazard and needs some cleanup. For instance, load file and switch to namespace do this. However, cider-eval-last-sexp only works in the current connection and is not mapped across both.

[dpsutton]

And I misspoke in the earlier comment. It certainly does not introduce a new type of repl, it just recognizes a new type of buffer, a cljc one. In that case, when looking for the current cider connection, it just returns the first one, and then if you are mapping across connections, you should get the other one from cider-other-connection

[dpsutton]

Just to prove my point, notice the implementation of changing namespace:

(defun cider-repl-set-ns (ns)
  "Switch the namespace of the REPL buffer to NS.

If called from a cljc or cljx buffer act on both the Clojure and
ClojureScript REPL if there are more than one REPL present.

If invoked in a REPL buffer the command will prompt for the name of the
namespace to switch to."
  (interactive (list (if (or (derived-mode-p 'cider-repl-mode)
                             (null (cider-ns-form)))
                         (completing-read "Switch to namespace: "
                                          (cider-sync-request:ns-list))
                       (cider-current-ns))))
  (when (or (not ns) (equal ns ""))
    (user-error "No namespace selected"))
  (cider-map-connections
   (lambda (connection)
     (cider-nrepl-request:eval (format "(in-ns '%s)" ns)
                               (cider-repl-switch-ns-handler connection)))
   :both))

and contrast with cider-interactive-eval

(defun cider-interactive-eval (form &optional callback bounds additional-params)
  "Evaluate FORM and dispatch the response to CALLBACK.
If the code to be evaluated comes from a buffer, it is preferred to use a
nil FORM, and specify the code via the BOUNDS argument instead.

This function is the main entry point in CIDER's interactive evaluation
API.  Most other interactive eval functions should rely on this function.
If CALLBACK is nil use `cider-interactive-eval-handler'.
BOUNDS, if non-nil, is a list of two numbers marking the start and end
positions of FORM in its buffer.
ADDITIONAL-PARAMS is a plist to be appended to the request message.

If `cider-interactive-eval-override' is a function, call it with the same
arguments and only proceed with evaluation if it returns nil."
  (let ((form  (or form (apply #'buffer-substring bounds)))
        (start (car-safe bounds))
        (end   (car-safe (cdr-safe bounds))))
    (when (and start end)
      (remove-overlays start end 'cider-temporary t))
    (unless (and cider-interactive-eval-override
                 (functionp cider-interactive-eval-override)
                 (funcall cider-interactive-eval-override form callback bounds))
      (cider-map-connections #'ignore :any)
      (cider--prep-interactive-eval form)
      (cider-nrepl-request:eval
       form
       (or callback (cider-interactive-eval-handler nil bounds))
       ;; always eval ns forms in the user namespace
       ;; otherwise trying to eval ns form for the first time will produce an error
       (if (cider-ns-form-p form) "user" (cider-current-ns))
       (when start (line-number-at-pos start))
       (when start (cider-column-number-at-pos start))
       additional-params))))

This functionality does not map across connections, which is why there was a toggle connections in the first place. If we want to open an issue about this I can work on making this more uniform. And in that case, we would need to come up with a way to limit the connections seen by the scratch buffer.

[bbatsov]

One problem is that this seems to be very haphazard and needs some cleanup. For instance, load file and switch to namespace do this. However, cider-eval-last-sexp only works in the current connection and is not mapped across both.

Yeah, this is a major problem that has to be fixed.

This functionality does not map across connections, which is why there was a toggle connections in the first place.

As I said - connection toggling was added mostly for the sake of the scratch buffers. I didn't expect that people would be using this to force the connection of a source buffer.

If we want to open an issue about this I can work on making this more uniform. And in that case, we would need to come up with a way to limit the connections seen by the scratch buffer.

Absolutely. Although I don't think limiting the connections seen by the scratch buffer is that important. Maybe simply it can prompt for a connection to use when it's created. This would make everything so much simpler...

[bbatsov]

As I said - connection toggling was added mostly for the sake of the scratch buffers.

Actually this is not true - now I remembered it was added as work-around for the difference in the eval commands behaviour and eventually I thought it might solve the problem with scratch buffers being Clojure only. The support for cljc in CIDER is truly a mess right now...

[bbatsov]

Same applies for clojurescript-mode. :-)

[bbatsov]

clj mode -> clojure-mode

[bbatsov]

cljc mode -> clojurec-mode

[bbatsov]

You forgot a closing paren here.

[bbatsov]

I'd simply use cons here.

[bbatsov]

Repl -> Connection

cider-connections -> conns

[bbatsov]

That's an odd change, btw - you need to make the other connection the default for this buffer (that's why the command is named toogle-connection. Your change simply rotates the connection...

[bbatsov]

Looking more closely at the PR - I really don't think you can toggle between a clj and a matching cljs connection simply by reordering the connection list the way you've changed the implementation. You have to find the matching connection and place it first for this to make sense. And in general code that relies on the connection order is super brittle and hard to understand. Apart from assuming that first connection is the default/active/most recent/whatever connection, I'd try not to make more guesses about the nature of a connection from its position in a list - that's simply too opaque for people trying to understand the code.

[dpsutton]

I think when I was making it I was assuming there were only connections for the particular project in the cider-connections var, and there would only be 2 at most. I'll have to go back to the drawing board then.

[dpsutton]

I guess my thinking is to set a buffer local var in cljc buffers of preferred type. And in cider-current-connection, if deriving from clojurec-mode, use this preference as the type preference in cider-current-connection.

[bbatsov]

This sounds reasonable.

[bbatsov]

Alternatively the first time you evaluate something in a cljc buffer we can prompt the user for a connection to select (in the presence of a clj and cljs connection for this project). Afterwards the user should toggle connections manually when they need to (or we'll get to a point where all eval commands dispatch over both connection). Btw, we should probably make this eval dispatch over both connections configurable as I'm not certain all users would appreciate/want it.

[dpsutton]

Btw, we should probably make this eval dispatch over both connections configurable as I'm not certain all users would appreciate/want it.

I think I like this, as when I was picking up Reagent, a cljs framework, loading namespaces caused errors in the clojure repl's and threw me off for quite a while.

[dpsutton]

I think I want to separate cider-map-connections from determining the connections to use. Right now, mapping connections attempts to determine the connections that you want. I would rather it behave more like a standard map operation in that the caller can determine which connections it wants, and then map some functions over those connections.

[bbatsov]

Makes sense to me.

[dpsutton]

This is obviously a failed first attempt. I'm gonna close it and think about it some more. I think the first step is tracking down every entrance to eval that CIDER provides and making sure they are all uniform.