-
-
Notifications
You must be signed in to change notification settings - Fork 651
[Fix #3834] Extend cider-repls to filter out REPLs that don't support needed operations #3840
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
[Fix #3834] Extend cider-repls to filter out REPLs that don't support needed operations #3840
Conversation
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. |
6d723a1 to
23b2c17
Compare
I think this solution would be optimal, I'll try implementing it! |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition seems outdated by now, maybe leaving it as before would be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantatic work, thanks!
Just be sure to update the changelog, PR description, and please add tests (especially if there was existing coverage in these areas)
aea853d to
8a2db0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
test/cider-connection-tests.el
Outdated
| :repl-type cider-connection-tests-dummy-function)) | ||
| :to-equal "*cider-repl me/myproject:localhost:12345(cider-connection-tests-dummy-function)*"))) | ||
|
|
||
| (describe "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally missed this 😅
8a2db0d to
b9e244e
Compare
|
Ready from my side, feel free to have one last look @bbatsov |
cider-connection.el
Outdated
| :initial-value '())) | ||
|
|
||
| (defun cider-repls (&optional type ensure) | ||
| (defun cider-repls (&optional type ensure ops-to-support) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better name for this would be "required-ops" or something along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes more sense, changed.
…d ops, use the new format also in cider-toggle-trace-ns
a346eeb to
96decbe
Compare
CHANGELOG.md
Outdated
| - Bump the injected `piggieback` to [0.6.1](https://github.com/nrepl/piggieback/blob/master/CHANGES.md#061-2025-12-31). | ||
| - Bump the injected `cider-nrepl` to [0.58.0](https://github.com/clojure-emacs/cider-nrepl/blob/master/CHANGELOG.md#0580-2025-10-16). | ||
| - * [cider-nrepl#951](https://github.com/clojure-emacs/cider-nrepl/pull/951): Debug: correctly process #dbg tag during load-file. | ||
| - [#3834](https://github.com/clojure-emacs/cider/issues/3834): Change cider-ns-refresh to always use Clojure REPL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget the final . here.
test/cider-connection-tests.el
Outdated
|
|
||
| (describe "killed buffers" | ||
| (it "do not show up in it" | ||
| (it "do not show up in it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems you've pressed RET accidentally here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed these, thanks!
96decbe to
4daa968
Compare
|
@ikappaki If you could take a look what's going on with the Windows build that'd be appreciated! (seems the deps installation is not working for some reason) |
|
Hi @bbatsov, I’ll take a look. Unfortunately, I can’t reproduce the issue locally, it’s likely related to GPG keys. Maybe we’ll get lucky and updating to 30.2 will resolve it ... |
|
(Fixed in #3845) |
Extend
cider-replsto take an additionalops-to-supportparameter, which is used to filter out the REPLs that do not support the given operations.Change
cider-map-replsto allow thewhich-parameter to be a list, that has the type of the wanted REPL as the first element and the rest as names of operations that need to be supported for thefunctionto work. These operations are then passed tocider-replsas theops-to-support.Change
cider-ns-refreshandcider-toggle-trace-nsto use the new list parameter forcider-map-repls. This fixes #3834.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.