-
-
Notifications
You must be signed in to change notification settings - Fork 645
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
New connection API and user level connection commands #2069
Conversation
8e4cd3b
to
6a47ee4
Compare
This is really great, so CIDER will have a good support for multiple projects in parallel now? |
The main motivation for this is to make codding with connections simpler with well defined semantics. But yes, the part concerning UI aims to give full control over project<->connections associations to the user with minimal hustle. |
6a47ee4
to
a1184b2
Compare
New variables: cider-connection-alist, cider-connection-name, cider-connected-directories New connection API functions: cider-delete-connection-repl, cider-add-connection-repl, cider-get-connection, cider-project-connections-repls, cider-current-connection-repls, cider-current-connection-repl New commands for connection association: cider-assoc-buffer-with-connection cider-assoc-directory-with-connection cider-assoc-project-with-connection cider-assoc-with-connection New semantics: cider-assoc-project-with-connection, cider-connections, cider-current-connection, cider-project-connections Removed: cider-request-dispatch, cider-connections, cider-find-connection-buffer-for-project-directory, cider-toggle-buffer-connection, cider-clear-buffer-local-connection, cider-toggle-request-dispatch, cider-current-connection, cider--in-connection-buffer-p, cider-default-connection, cider-rotate-default-connection cider--guess-cljs-connection, cider--has-warned-about-bad-repl-type, cider-map-connections, cider-connections-make-default, cider--connections-make-default, nrepl-use-this-as-repl-buffer Work, but need to be re-factored/adjusted: cider-change-buffers-designation cider-connection-browser
a1184b2
to
5dc8fad
Compare
I'll try find some time to review this next week. @expez @benedekfazekas You should review it carefully too, as such an extensive change is going to break clj-refactor for sure. (if we decide to go with it) |
Just a few thoughts - if we're doing something so extensive I think that's the time to decouple the concept of a CIDER connection from the concrete REPL provider (e.g. nREPL). Ideally connections should have a type (nREPL, socket, unrepl) and based on the type some different underlying repl-specific code would get invoked (e.g. code from nrepl.el). This ties with something we've discussed a while ago - decoupling CIDER from nrepl.el. |
Probably not that much. It's more of a conceptual and API cleanup PR, not a fundamental change in how Cider works. If the only internal api that clj-refactor was using is
nREPL interface is at a lower level than what I was concerned with in here. I am dealing with user visible connections, not nREPL sockets. The user visible connection is now a set of nREPL connections and I call them REPLs for the lack of a better term (suggestions welcome), but they can be any kind of objects in the future - REPL buffers, nREPL session, sockets of any kind etc. If the user level connection concept and associated API are encapsulated, the actual data structure can be changed in whatever way needed in the future without affecting the user. The generic nREPL interface is outside of this PR and will likely take a much longer to figure out.
Would it be possible to limit activity on cider-client.el, cider-interaction.el, cider.el and cider-repl.el a bit? Even small changes are now likely to introduce hard to fix conflicts on my side. |
(message "Waiting for CIDER connection %s to quit..." | ||
(cider-propertize buf-name 'bold)) | ||
(sleep-for 2) | ||
(pcase conn-creation-method |
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.
CLJS support for restart hasn't worked in the past since cljs repls are started with a callback that's not recorded. Ie, when you call restart, the cljs repl is recreated as the clj repl that it began as. Since we are adding some more information, can we do 1 of two things here?
- keep a reference to the initial callback that created the cljs and call that callback again or
- record that the cljs repl will be recreated again if the original is restarted. So in this case it is marked as a side effect of the original so when restarting, the cljs buffer is destroyed and the original clj buffer will create it again. I think in
cider-jack-in
differs fromjack-in-clojurescript
solely by passingcljs-too
as a truthy value.
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 haven't touched restart yet but I am aware of the problem. If CLJS+CLJ+etc are treated as a unit (as intended by this PR) start/close/restart must be applied on both simultaneously. Your nr 2 is probably the way to go. Every relevant parameter must be preserved in order to recreate the entire connection (in the new sense). Once the dust is settled I will make sure this part works as expected.
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.
awesome. if you get this first pass stable enough to push here I'm happy to divide issues up with you and start improving. I just don't want to touch anything right now while you shuffle things. Love the scope of things you are doing though
|
||
(defun cider-assoc-buffer-with-connection () | ||
"Associate the current buffer with a connection. | ||
(defun cider-repl-buffers () |
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.
why doesn't this function use the connection a-list?
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's a legacy function, I just moved it around. In theory all REPL buffers should be in the conneciton-alist and the other way around. If relied on connection-alist and to conform to new naming convention, this function should be named cider-connections-repls
meaning "all repls from all connections".
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 like the new naming convention; perhaps we candefine-obsolete-variable-alias
?
can we put this on a branch on the clojure-emacs repo of cider so we can play with this? I'd like to look into this more. In particular, I'm starting a cljs job this thursday and I want to make sure that cljs support stays first class. |
Yeah, absolutely. |
A small update on this. My earlier decision to use "connection" to mean the "user visible connection" was mostly due to unavailability of a more suitable name for this notion. Now I think that "user session" is a more appropriate one. The reason I didn't use "session" earlier is the perceived clash with nrepl sessions. But nrepl sessions are low level internal wirings which surface in cider only in a few places, and sometimes for wrong reason. For instance |
Small update from me as well - now that 0.15.1 is out I want us to wrap your other two simpler PRs and I'll focus on this one. I'd appreciate it if more people looked into this. Ideally we'd cut 0.16 quickly with just those major changes, so people can benefit from them faster. |
If we can merge this onto a branch on the clojure-emacs cider, i'm happy to use that branch at work to give feedback and pain points |
I think that such testing is pointless before settling on whether the new API is what we need. I should have more time for this next week and I hope that by then we would have wrapped the other PRs by @vspinu so we can all focus on this one. |
Any news on this one? Is there anything I can do to help? I am not particularly good with Elisp, but if you are in need of tests (I have clj-projects, cljs-projects and clj-cljs-projects available), ping me. :) |
I was overwhelmed with work and I've yet to start any serious work here. My workload should supposedly go down in the next couple of weeks, so I'm hopeful there'll be some development here soon. |
(let ((project-directory (file-truename project-directory))) | ||
(seq-filter (lambda (conn) | ||
(when-let ((buff (cadr conn))) | ||
(when-let ((conn-proj-dir (with-current-buffer buff |
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.
when-let
(now when-let*
per #2132) takes multiple bindings.
|
||
(defun cider-project-connections-repls (&optional project-directory) | ||
"Return all REPL buffers for PROJECT-DIRECTORY." | ||
(apply #'append (mapcar #'cdr (cider-project-connections project-directory)))) |
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.
(apply #'append (mapcar ...))
-> mapcan
(apply #'append (mapcar #'cdr (cider-project-connections project-directory)))) | ||
|
||
(defun cider-project-connections-types (&optional project-directory) | ||
"Return a list of types of connections within PROJECT-DIRECTORY." |
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.
Since project-directory
is optional, would be good to mention default value / behavior in docstring.
(let ((dir (file-truename (or dir default-directory))) | ||
(match "") | ||
(conn nil)) | ||
(maphash (lambda (k v) |
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'm not sure what this is trying to do, but cl-loop
may be cleaner 🤷♂️.
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.
Yeah, this code is a bit dense.
(seq-filter right-type-p (cdr conn)) | ||
(cdr conn)))) | ||
|
||
(defun cider-current-connection-repl (&optional type) |
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 this method name is misleading given what the function actually down.
Overall the implementation seems reasonable to me. I've left a few inline remark, and @xiongtx also made some good suggestions. My only bigger concerns are the use an of Anyways, looking forward to hearing from you. Sorry about the crazy turnaround on this - now that I read the code I saw your changes were actually quite simple and we could have merged them a while ago (although they'll still be pretty painful for all CIDER extensions). |
cider-connections | ||
(car cider-connections))))) | ||
|
||
;;; Connection Accessors | ||
|
||
(defun cider-connection-type-for-buffer () |
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.
Would be nice for the function to take an optional buffer parameters, which defaults to current-buffer
. I find this "optionally functional" style easier for testing.
@vspinu One more thing. Once you say you're working on updating this PR to get it to a mergeable state I'll halt work on everything else to make sure you don't have to deal with more merge conflicts. |
alists are safer regarding copying and buffer-local behavior. The default api for alists is as versatile as for hashtables and readability is better. So for small use cases like this I would choose alists.
There are two ways of going about it, one is centralized where a global alist contains all the associations (directories/projects/buffers) and another decentralized when the global alist contains only the connections proper with associated names. In the decentralized approach one would use a separate alist per association type (one for directories, one for projects etc.) I chose the decentralized approach because it seems simple, separates concerns and allows for further extensions without changing the core data structures. Who knows what that connection alist will be used for in the future.
The data structure itself is not restrictive. Values in the alist are arbitrary and could change without affecting user level API. Currently values are buffers, but I would like to have processes there eventually. General connections need not have associated buffers and one REPL buffer might some day be associated with multiple processes.
I am very busy till 12th but will give it a shot immediately after. As you pointed out, things are not that complicated and I hope to be able to isolate the connection management part in a small reusable library. |
OK, I can live with that. I see your point even if a prefer naming attributes in any abstraction explicitly, so it's more clear to users what they are dealing with. On the other hand assuming that most people won't actually touch the alist directly they'll get most of that info from the API functions.
Sounds like a plan to me. The sooner we get this over with, the better! |
Friendly ping to know how this is evolving. :-) |
Seems @vspinu is still busy. I'm just as eager as everyone else to press the magic merge button and release CIDER 0.17. |
I am slowly working on it. Cannot put in too many hours at the moment but likely within a couple of weeks will have something ready for a review. |
@bbatsov, if this is the only patch that holds 0.17 from happening then it might be better to release it without. While I am reasonably confident that I will have the generic session manager out in a week or two, the discussion might take a week and the actual integration into CIDER (if the system is accepted) yet another week. So I don't see the integration happening earlier than a month from now. |
It's not the only thing remaining - there's also a bit more work on orchard and making it possible to use It'd be nice if we squeezed the new connection API in, but if we don't - we'll cut 0.18 just with it as the only change. |
sesman is here! General discussion on the first issue vspinu/sesman#1. I will wait for comments and suggestions and, if all goes well, will propose integration patch by the end of the month. |
I guess at this point we can close this. If we don't make progress with sesman I'll write something similar to this PR for the next version of CIDER myself. I've long delayed working on this as I hoped we'd simply end up using @vspinu's work. |
Not sure what you mean by this. Are you unhappy about lack of development on sesman in the last 2 months? 2 months is surely not "long" in my world. You never said you plan to work on this part, nor you let me know when I should expect a slow down in CIDER development. Back in march CIDER was going through a storm of development, so I assumed a good time would be after the release, which, well, hasn't happened yet. |
No, no. I'm not unhappy with you in any way. I know how life and responsibilities work.
Well, I always planned to work on this, but I hoped you'd do the work so I won't have to. :D Recently the connection management has become more and more problematic with the rise of ClojureScript and everything currently's so messed up, that fixing this has become one of my top priorities for the next release. I'll cut the new release on Monday, btw. Let me know if you'd like to do the connection management yourself in the next release or if I should cook something myself. |
Cool. I will start working once the release is out then. |
… On 7 May 2018 at 13:51, Vitalie Spinu ***@***.***> wrote:
I'll cut the new release on Monday, btw.
Cool. I will start working once the release is out then.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2069 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGVyhuQ8UMsLxGqOgRnl_x5aBnjzYG7ks5twCdIgaJpZM4OrZef>
.
--
Best Regards,
Bozhidar Batsov
http://www.batsov.com
|
@vspinu I'm just checking in regarding this. Last week you were optimistic you'd finish it quite soon. :-) |
I am almost there, and to my mind it's much better than the original PR :) I ended up rewriting the entire jack-in/connect system on this occasion and still have to figure out a few things. Will be back by the end of the week with a fully functional proof of concept. |
Awesome! |
I am spending 95% of time being 95% ready at the moment. The cider side is here and it's "mostly" working, but I haven't tested far beyond the life-cycle system as yet. Will need a few more days to polish the PR and document the new system. |
That sounds pretty familiar. 😆
Thanks for the update! I'll take a look at the current state of affairs. |
Second attempt on #2069. A brief description of the new functionality follows. 1) __Jack-in/connect__ - User level commands: Create new sessions: C-c M-j: cider-jack-in-clj C-c M-J: cider-jack-in-cljs C-c M-c: cider-connect-clj C-c M-C: cider-connect-cljs Add new REPLs to the current session: C-c M-s: cider-connect-sibling-clj C-c M-S: cider-connect-sibling-cljs - `cider-jack-in-clojurescript` no longer creates two repls, only the cljs repl - clj repl has no longer a special status of the "main" repl. All repls within a session share same server and are siblings of each other. You can create as many clj and cljs siblings as you want from any repl. - Creation of the client is no longer tightly bounded with the nrepl-server startup. The dynamic communication mechanism between jack-in and nrepl-server filter has been replace by a simple `on-port-callback`. 2) __New Connection and Session Management API__ - Connections (aka REPLs) are grouped in [sesman](https://github.com/vspinu/sesman) sessions. Sibling repls are added to the current session. - Cider connection commands (`cider-quit`, `cider-restart`, `cider-display-connection-info`) have been refactored to operate exclusively on the connection level. - Sesman commands operate on the whole session: ![sesman-map](https://user-images.githubusercontent.com/1363467/41355277-6864ffb8-6f21-11e8-9387-3de586477d68.png) - Associations (links) between current context (buffer, directory, project) are governed by sesman and could be formed only on the session level. In a nutshell, session can be linked to projects, directories and buffers. Buffer link have precedence over directory links, and directory have precedence over project links. When a sesman session is registered it's automatically linked with the lowest priority context (project, or directory if no project found). By default (configured with `sesman-1-to-1-links`) multiple sessions can be linked with a project or a directory, but only one session can be linked with a buffer. Cider functionality (eval, completion, repl-switching etc) operate on linked sessions. When there are multiple linked sessions ambiguity is automatically resolved by the recency of the REPL buffers (configured with `sesman-disambiguate-by-relevance`). - Show info on current links with `C-c C-s l`. Show info on current, linked or all sessions with `C-c C-s i`. - Micro-management of the server is not allowed (it's not useful and would complicate UI). All repls within a session share a server. Server can be either remote (`cider-connect`) or local (bootstraped within the emacs during `cider-jack-in-xyz`). In case of the local server, when the last connection is killed the server is automatically killed. `cider-restart` restarts the connection but not the server. `sesman-restart` restarts the server and all the connections. At least two issues I still plan to tackle here: - Restart of SSH tunneled connection has not been tested and probably doesn't work - REPL buffer naming system is no longer adequate. It should be possible to include session name as part of the buffer name and add more flexibility into the customization of buffer name templates. ----- A tot of no longer necessary or questionable functionality has been removed. The goal is to start from scratch and add only what is really necessary. I am listing all the removed functions for the ease of lookup through the github interface. Removed: cider--connection-host, cider--connection-port, cider--connection-project-dir, cider--connection-properties, cider--connection-type, cider--guess-cljs-connection, cider--has-warned-about-bad-repl-type, cider--in-connection-buffer-p, cider--quit-connection, cider--restart-connection, cider-assoc-buffer-with-connection, cider-assoc-project-with-connection, cider-change-buffers-designation, cider-clear-buffer-local-connection, cider-close-nrepl-session, cider-connections (variable), cider-current-connection (variable), cider-current-messages-buffer, cider-current-repl-buffer, cider-default-connection, cider-extract-designation-from-current-repl-buffer, cider-find-connection-buffer-for-project-directory, cider-find-reusable-repl-buffer, cider-make-connection-default, cider-map-connections, cider-other-connection, cider-project-connections, cider-project-connections-types, cider-prompt-for-project-on-connect, cider-read-connection, cider-repl-buffers, cider-replicate-connection, cider-request-dispatch, cider-rotate-default-connection, cider-toggle-buffer-connection, cider-toggle-request-dispatch, nrepl-connection-buffer-name-template, nrepl-create-client-buffer-function, nrepl-post-client-callback nrepl-prompt-to-kill-server-buffer-on-quit, nrepl-use-this-as-repl-buffer, Connection Browser Functionality: cider--connection-browser-buffer-name, cider--connection-ewoc, cider--connection-pp, cider--connections-close-connection, cider--connections-goto-connection, cider--connections-make-default, cider--connections-refresh, cider--connections-refresh-buffer, cider--ewoc-apply-at-point, cider--setup-connection-browser, cider--update-connections-display, cider-client-name-repl-type, cider-connection-browser, cider-connections-buffer-mode, cider-connections-buffer-mode-map cider-connections-close-connection, cider-connections-goto-connection, cider-connections-make-default, cider-display-connected-message, cider-project-name, Renamed: cider-current-session -> cider-nrepl-eval-session cider-current-tooling-session -> cider-nrepl-tooling-session cider-display-connection-info -> cider-describe-current-connection cider-create-sibling-cljs-repl -> cider-connect-sibling-cljs nrepl-connection-buffer-name -> nrepl-repl-buffer-name cider--close-connection-buffer -> cider--close-connection ## repl <> connection overlap cleanup cider-connections -> cider-repls cider-current-connection -> cider-current-repl cider-map-connections -> cider-map-repls cider-connection-type-for-buffer -> cider-repl-type-for-buffer cider-repl-set-type -> cider-set-repl-type
Attempting to bootstrap the connection API . I guess we could all agree by now that the current status is rather unsatisfactory, so I am skipping the introduction.
It's probably best to review the new code directly rather than from the diff. The key points are:
Most of the issues with connections are due to the lack of an intermediate "connection unit" in between nREPL sessions (or REPL buffer) and project directory. To fix that, one could either introduce a new unit directly or re-brand the old name into a new concept. I have opted for the second as the change should then be transparent to the user.
So, there is now a new "connection" unit defined as a collection of nREPL transport processes. As nREPL process in Cider is confounded with the corresponding REPL buffer the new "connection" is a structure with a name and a bunch of REPL buffers. This is the user visible connection unit. Most user interactions should operate on this unit (assoc connections and interactive eval being the most important).
The connection API consists of a few functions to add/delete and retrieve relevant REPLs from
cider-connection-alist
. Most notablycider-current-connection
has now a clear semantics which allows for buffer local association, directory association and project level associations. There is now very little room for sending code to wrong connections.The user level association API consists of three commands for buffer, dir and project associations and one DWIM command for most common case. There is a need for a one or two
dissoc
commands which I left for a later date. I still need to add a proper md doc entry for this part, but hopefully the code is sufficiently clear.I have removed the old
static
connection API altogether. Life is too short to maintain two parallel APIs, let alone that that system was a pretty awful UI to begin with.I have also added a bunch of FIXMEs in the code. Those are not regressions but probable bugs in current implementation mostly concerning
cljc
interaction. Basically, every occurrence ofcider-current-connection
in the old code (cider-current-connection-repl
in the new version) is likely to be doing something wrong forcljc
setup. Those should be tackled on case by case, but are relatively straightforward to fix within the new framework.New variables:
New connection API functions:
New commands for connection association:
New semantics:
Removed:
Work, but need to be re-factored/adjusted: