Reify cider-cljs-pending separate from repl-type #3291
Conversation
benjamin-asdf
force-pushed
the
cider-cljs-pending-suggestion
branch
from
a8b5c40
to
254fe9a
Compare
Pretty sure that's not an use-case we should worry about, as I can't see why someone would be doing this. Your proposed solution is reasonable. I agree that the pending upgrade status is not really a REPL type, but rather an internal implementation detail. |
cider-repl.el
Outdated
| @@ -224,6 +224,8 @@ This cache is stored in the connection buffer.") | |||
| (nrepl-dbind-response response (repl-type changed-namespaces) | |||
| (when (and repl-type cider-repl-auto-detect-type) | |||
| (cider-set-repl-type repl-type)) | |||
| (when (string= repl-type 'cljs) | |||
There was a problem hiding this comment.
You should use string= to compare something with a symbol - eq is what you're looking for here.
There was a problem hiding this comment.
I tried this with eq first. repl-type is a string at that point. Could instead call cider-maybe-intern on the type and say eq ?
There was a problem hiding this comment.
(updated version seems better)
cider-connection.el
Outdated
| @@ -762,10 +762,17 @@ Session name can be customized with `cider-session-name-template'." | |||
| (defvar-local cider-repl-type nil | |||
| "The type of this REPL buffer, usually either clj or cljs.") | |||
|
|
|||
| (defvar-local cider-cljs-repl-pending nil | |||
There was a problem hiding this comment.
I think this might be a good time to change the terminology to the more descriptive cider-repl-cljs-upgrade-pending. This will make the code more readable across the board.
|
The proposed change needs a changelog entry as well, as we're removing a REPL type (even if it was kind of fake). |
| @@ -224,6 +224,8 @@ This cache is stored in the connection buffer.") | |||
| (nrepl-dbind-response response (repl-type changed-namespaces) | |||
| (when (and repl-type cider-repl-auto-detect-type) | |||
| (cider-set-repl-type repl-type)) | |||
| (when (eq (cider-maybe-intern repl-type) 'cljs) | |||
There was a problem hiding this comment.
Ah, if that's a string than the comparison should be (string= repl-type "cljs"). I just saw 'cljs was a symbol and I assumed that repl-type was a symbol as well.
|
I'll merge this as is and I'll make a couple of small tweaks myself, so I won't waste your time with them. |
This pull request addresses #3287 by separating the concept of a repl type from the concept of pending status. The pending variable is now set when the repl is initialized, and it is set to
nilwhen we are sure that we have a cljs repl incider-repl--state-handler.In
cider-repls, we no longer return pending repls, which is the same semantic as before (calling(cider-repls 'cljs) or (cider-repls 'clj) or (cider-repls nil)does not return pending repls. The only potential use case that is broken with this change is if a user has written an emacs lisp function that calls(cider-repls 'pending-cljs). We could add a legacy special case for this, where calling cider-repls with 'pending-cljs returns the repls that arecider-cljs-pending-p.Overall, this change will prevent future headaches by clearly separating the concepts of repl type and pending status.