[Fix #3834] Change cider-ns-refresh to use Clojure REPL whenever one exists #3840
Conversation
wilbrt
force-pushed
the
3834-ns-refresh-to-use-clj-repl
branch
from
90b2135 to
6d723a1
Compare
|
Hmm, seems to me something's wrong with |
|
There doesn't seem to be anything wrong with I thought about extending the |
Well, it seems to me that we should solve that root cause then. Would moving the ...If it didn't , maybe we could try adding a parameter so that the connection being looped over is relayed. |
|
Thinking about it, a nice API addition for - (cider-map-repls :clj (lambda (...
+ (cider-map-repls '(:clj "refresh" "cider.clj-reload/reload") (lambda (...That would-be variant would loop over the clj connections that had those specific capabilities, without throwing an error in non-matching connections. |
wilbrt
force-pushed
the
3834-ns-refresh-to-use-clj-repl
branch
from
6d723a1 to
23b2c17
Compare
I think this solution would be optimal, I'll try implementing it! |
wilbrt
force-pushed
the
3834-ns-refresh-to-use-clj-repl
branch
from
23b2c17 to
f65ea20
Compare
The latest commit presents a proposal for the kind of API mentioned in your comment. Though I was not sure what to do about the |
cider-connection.el
Outdated
| (t 'ensure))) | ||
| (repls (cider-repls type ensure))) | ||
| (mapcar function repls)))) | ||
| (mapcar (lambda (repl) |
There was a problem hiding this comment.
This throws an error for non-matching connections, which seems undesirable.
My suggestion was that the ops-to-support act as a 'filter', selecting the acceptable repls that we'll iterate over.
Taking a look at the function body, it seems to me that the cleanest approach would be to:
- also introduce a
ops-to-supportparameter to thecider-replsfunction - relay that parameter from here
This way, we still have the flexibility to be 'strict' (error-throwing) or not, by reusing the existing ensure logic.
There was a problem hiding this comment.
I hope I understood the idea correctly 😅 . It seems that at least the cider-toggle-trace-ns uses the cider-map-repls with cider-ensure-op-supported inside the lambda, do you think that that should be changed to be inline with the new API as part of this, as part of another task or not at all?
There was a problem hiding this comment.
It seems that at least the cider-toggle-trace-ns uses the cider-map-repls with cider-ensure-op-supported inside the lambda, do you think that that should be changed to be inline with the new API as part of this, as part of another task or not at all?
Seems okay to include as part of this PR, provided that you try it out
| | `cider-ns-refresh` | ||
| | kbd:[C-c M-n (M-)r] | ||
| | Reload all modified files on the classpath. If invoked with a prefix argument, reload all files on the classpath. If invoked with a double prefix argument, clear the state of the namespace tracker before reloading. | ||
| | Reload all modified files on the classpath. If invoked with a prefix argument, reload all files on the classpath. If invoked with a double prefix argument, clear the state of the namespace tracker before reloading. Uses a clojure REPL whenever one exists. |
There was a problem hiding this comment.
This addition seems outdated by now, maybe leaving it as before would be enough
Change
cider-ns-refreshto use a Clojure REPL whenever one exists, by wrapping the function body in awith-current-bufferwith the clj-repl connection if it exists. If not, throw ano-repls-user-error. Fixes #3834.Not sure if this is a reasonable way to handle this, so all critiques and comments are more than welcome, thanks!
Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
eldev test)eldev lint) which is based onelisp-lintand includescheckdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.Thanks!
If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.