[Fix #2466] Make generic cider ops use any available nrepl connection #2467
Conversation
Congratulations! Thanks for the nicely executed PR! Hopefully it's going to be the first of many - we sure need more people to help out! |
cider-client.el
Outdated
| @@ -533,7 +533,7 @@ The result entries are relative to the classpath." | |||
| "Perform nREPL \"format-code\" op with CODE." | |||
| (thread-first `("op" "format-code" | |||
| "code" ,code) | |||
| (cider-nrepl-send-sync-request) | |||
| (cider-nrepl-send-sync-request (cider-current-repl "clj")) | |||
There was a problem hiding this comment.
Looking at the other code in the file I think of two things:
- what happens if you only have a cljs connection? Seems to me the formatting should still work, as it's not really clj or cljs specific
- I see we're utilizing the same pattern for other ops as well, which means they would likely exhibit the same problem as the formatting issue
There was a problem hiding this comment.
what happens if you only have a cljs connection? Seems to me the formatting should still work, as it's not really clj or cljs specific
True. I didn't consider having just a cljs repl to be a thing.
So I guess "multi" would be the right thing to pass to cider-current-repl?
I see we're utilizing the same pattern for other ops as well, which means they would likely exhibit the same problem as the formatting issue
Perhaps, yes. I just investigate since I didn't run into any of those other issues yet.
There was a problem hiding this comment.
Just tested using (cider-current-repl "multi") and it does make it work with either an cljs or an clj repl active.
There was a problem hiding this comment.
re other ops: additionally to change other ops, utility functions like cider-nrepl-op-supported-p would also have to be changed.
|
@bbatsov would you prefer closing this PR in favor of a more thorough one addressing this in a general fashion for all ops? |
|
We'll certainly need a generic solution for this, but I'll have to think a bit about what makes the most sense. I think historically we sent all middleware evaluations to the Clojure REPL, as before 0.18 it wasn't possible to have just a ClojureScript REPL. @vspinu What do you think about this? |
|
Sorry for the late reply (I am on vacation atm). I think any connection would do for the purpose of format and other generic ops. Hence "multi" seems like the right approach. Unfortunately in the context of |
I like this! |
|
@bbatsov @vspinu I've pushed an attempt to as you suggested. As eval uses it's connection selection, it seemed safe to me to just adjust the What do you think? |
kommen
changed the title
kommen
force-pushed
the
fix-2466
branch
3 times, most recently
from
2d7c8f3
to
fdd1c2c
Compare
| @@ -153,7 +153,7 @@ REQUEST is a pair list of the form (\"op\" \"operation\" \"par1-name\" | |||
| \"par1\" ... ). | |||
| If CONNECTION is provided dispatch to that connection instead of | |||
| the current connection. Return the id of the sent message." | |||
| (nrepl-send-request request callback (or connection (cider-current-repl)))) | |||
| (nrepl-send-request request callback (or connection (cider-current-repl "any")))) | |||
There was a problem hiding this comment.
Seems to me we should assume "any" by default.
|
Looks good to me overall. Just add some entry in the changelog for this. |
…pl connection Make "any" a synonym of "multi" for `cider-current-repl`, which makes it more clear any avaiable nrepl connection can be used. This allows ops for like documentatation browser to work, even if only a repl for a different type of repl is connected. For example, when formatting a .cljs buffer, a connected "clj" repl will suffice. Cider eval uses a separate repl connection selection, which lets it unaffacted by these changes.
|
@bbatsov thanks, I've added the changelog entry |
This makes formatting code always use the clj repl type, which makes the
cider-format-*commands work in cljs buffers even if there is no cljs repl active.Fixes #2466
[This is my first contribution to cider, discussion & feedback very welcome]