-
-
Notifications
You must be signed in to change notification settings - Fork 45
Include user function to create a socket repl inside emacs/inf-clojure #210
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Up to this point I've included pretty much all the features that I think are useful, hopefully all that is left now is making this PR consistent with the inf-clojure project/emacs etc. |
One thing that's not clear to me is why do we need a different handling for ClojureScript REPLs now? This seems orthogonal to the goal of having some way to start a socket REPL and connect to it. CIDER has taught me that trying to support the use-case of having multiple REPLs for the same project adds a ton of complexity (grouping REPLs, deciding where to dispatch evaluations), so I'm not sure I want us to go in this direction here as well. |
Thanks @bbatsov - It's probably my misunderstanding, with nREPLs you can have multiple sessions so it's possible to have a single nREPL that has a separate Clojure and ClojureScript session. I don't know how this is done with socket REPLs, as far as I can tell there is just a single 'session' making it difficult/impractical to work on applications with both frontend and backend. It's for this reason that I think different handling for ClojureScript REPLs is necessary. If there is a good way to do this, I'd be in favor of making changes to keep just a single REPL and simplify everything. |
Yeah, with the socket REPL there's no way to host different REPLs in the same server, that's why from the perspective of |
Hmm, perhaps a good way forward then would be to remove the ClojureScript specific REPL & logic. Instead, what do you think about this as an alternate suggestion, 2 defcustom variables:
This way, we keep the internal logic simple, there is only a single global REPL as before and no complication to users unless they choose to use the above 2 customisations. I realise this goes beyond the original PR topic of just simply adding a socket REPL, it's somewhat of a shame as having the ability to run a Clojure and ClojureScript REPL is almost necessary for my (and I assume other people's) workflow. If this suggestion is not suitable either, then just let me know and I'll just stick to adding the socket REPL helper function. |
Hey @bbatsov - I've gone ahead an removed all the cljs/dual REPL buffer stuff to keep this a simple PR in an effort to make this simpler 😂 Still open to doing the above alternate suggestion if you think it's a good fit but otherwise let's just get the original job done! |
We can discuss this afterwards. I'm a big believer at making changes one at a time, so we can best discuss them. |
inf-clojure.el
Outdated
@@ -176,6 +176,9 @@ | |||
Its root binding is nil and it can be further customized using | |||
either `setq-local` or an entry in `.dir-locals.el`." ) | |||
|
|||
(defvar-local inf-clojure-repl-name nil |
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 didn't find usages of this in your diff.
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.
Doesn't appear to be used, I'll remove this thanks!
inf-clojure.el
Outdated
(funcall inf-clojure-socket-callback))))))) | ||
|
||
(defun inf-clojure-repl-sentinel (process event) | ||
"This function is called on update change, it is mainly used to ensure the socket a REPL was connected to is cleaned up when the REPL buffer is closed." |
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 won't pass checkdoc
validation. You can use something like eldev lint
locally to discover such problems.
inf-clojure.el
Outdated
(babashka . "bb socket-repl %s"))) | ||
|
||
;;;###autoload | ||
(defun inf-clojure-socket (&optional &rest args) |
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'd probably name this inf-clojure-socket-repl
as it's a more descriptive name.
I'm wondering why you didn't list out the supported params explicitly and made it possible for the user to be prompted about things like host
and port
to use.
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've updated the name, thanks for this.
I've also changed the function signature to be cmd
and operate essentially the same as inf-clojure
. Choosing a port can be done with the custom variable inf-clojure-socket-repl-port
but otherwise it's an implementation detail and gets automatically chosen. The user will be prompted for a command to use (from the defined defaults in inf-clojure-socket-repl-startup-forms
) just like the main function too!
I did originally use the &rest/plist approach because there were a lot of variables that I thought users may want to override but now that the function is simpler, hopefully this is a good change.
inf-clojure.el
Outdated
(get-buffer-process (get-buffer (concat "*" repl-buffer-name "*"))) | ||
#'inf-clojure-repl-sentinel)))) | ||
(set-process-filter sock #'inf-clojure-socket-filter) | ||
(message "Starting socket server at %s:%s with '%s'" host port socket-cmd))) |
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'd use wording like Staring X socket REPL server at ...
.
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.
updated
@@ -117,6 +117,9 @@ common startup forms. You can select one of these or type in your own | |||
custom startup. This will start a REPL process for the current project | |||
and you can start interacting with it. | |||
|
|||
If you want to use a socket REPL server, use `M-x inf-clojure-socket` | |||
which will start a socket server and connect to it for you. |
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.
socket REPL server
I appreciate the idea behind making it easy to start a repl, but I'm not sure it is that great in practice. The cljs repl doesn't seem to start up a cljs socket repl. I'm not aware how to get one without using mfikes tubular which is a more complicated setup. Personally I use I'm a huge fan of the following shell script: # sample usage: clj -J"$(socket-repl 6000)" ... or java "$(socket-repl 6000) -jar ...
socket-repl() {
echo -n "-Dclojure.server.repl={:port $1 :accept clojure.core.server/repl}"
} Invoked like # socket repl with clj
clj -J"$(socket-repl 6002)"
# socket repl from a jar
java "$(socket-repl 6000)" -jar metabase.jar I then have the following two functions to connect to that repl: ;; start my work repl with the fipp pretty printer on port 50505
(defun personal/work-repl ()
"start a work repl on port 50505"
(interactive)
(let ((inf-clojure-custom-repl-type 'clojure))
(inf-clojure-connect "localhost" 50505))
(let ((requires "(require '[clojure.core.server :as server]
'clojure.main
'[clojure.string :as str]
'clojure.test
'[fipp.edn :as fipp])")
(sub-repl "(clojure.main/repl
:prompt (fn [] (printf \"%s=> \" (peek (str/split (str *ns*) #\"\\.\"))))
:eval (fn [f] (binding [clojure.test/*test-out* *out*] (eval f)))
:read server/repl-read
:print fipp/pprint)"))
(inf-clojure-insert-and-eval requires)
(sit-for 0.2)
(inf-clojure-insert-and-eval sub-repl)
(sit-for 0.2)
(inf-clojure-insert-and-eval ":ready")))
;; start a general repl on the supplied port. pretty printer is clojure.pprint/pprint
(defun personal/repl (port)
"Connect to a clojure repl at PORT."
(interactive "nRepl port: ")
(let ((requires "(require '[clojure.string :as str]
'[clojure.core.server :as server]
'[clojure.pprint :as pprint]
'clojure.test)"
)
(sub-repl "(clojure.main/repl
:prompt (fn [] (printf \"%s=> \" (peek (str/split (str *ns*) #\"\\.\"))))
:read server/repl-read
:eval (fn [form] (binding [clojure.test/*test-out* *out*] (eval form)))
:print clojure.pprint/pprint)"))
(let ((inf-clojure-custom-repl-type 'clojure))
(inf-clojure-connect "localhost" port))
(inf-clojure-insert-and-eval requires)
(sit-for 0.2)
(inf-clojure-insert-and-eval sub-repl)
(sit-for 0.2)
(inf-clojure-insert-and-eval ":ready"))) These connect to socket repl ports, start my preferred sub repls with clojure.main/repl, set the repl type, etc. I much prefer these easier routes over complicated session management. These tools are the base tools involved in our stack. Adding another layer with different defaults, different params, environmental variables, etc gets opaque and difficult to follow. I'm also a big fan of using the actual tool so that bug repos, working with remote repls, configuration is as simple as possible.
One of the things that I absolutely love about inf-clojure compared to CIDER is that there is always a single, global connection. This makes it trivial to send code in any buffer (regardless of project or even if it is backed by a file on disk) to the repl. There's a function I'm really hesitant about bringing a notion of session management into I frequently have several repls open at the same time. I could have a shadow socket repl, my main clojure development repl, and then one or two repls into release jars to compare behavior. Trying to manage sessions and which belong to the project just gets really annoying. And when there's a bug, figuring out why is often so complicated. |
inf-clojure.el
Outdated
;; comint adds the asterisks to both sides | ||
(repl-buffer-name (format "*%s*" process-buffer-name))) | ||
(repl-buffer-name (format "*%s*" process-buffer-name)) | ||
(repl-type (or (unless prefix-arg |
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 was repl-type
moved up? Is there a functional change there or just moving it up in the lexical scope?
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 was due to my earlier implementation, it's no longer required and has been reverted - thanks!
inf-clojure.el
Outdated
repl-buffer-name | ||
repl-buffer | ||
(get-buffer-create repl-buffer-name)) | ||
(inf-clojure-connect host port) |
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 seems to be the only usage of host
. So that means we always create the process on the local machine (that makes sense) but then we optionally try bind against a remote machine which seems wrong. If we accept a host, it seems like that information needs to be included when starting our socket repl? but I'm not clear what values host could be.
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.
host is now always "localhost", we're passing it in two places inf-clojure-connect
and a message
but not the actual socket startup as you pointed out - I think this makes sense now to keep as a let variable since it's not configurable.
inf-clojure.el
Outdated
@@ -932,7 +1070,7 @@ Indent FORM. FORM is expected to have been trimmed." | |||
(let ((clojure-process (inf-clojure-proc))) | |||
;; ensure the repl buffer scrolls. See similar fix in CIDER: | |||
;; https://github.com/clojure-emacs/cider/pull/2590 | |||
(with-selected-window (or (get-buffer-window inf-clojure-buffer) | |||
(with-selected-window (or (get-buffer-window (inf-clojure-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.
You are invoking inf-clojure-buffer
. But that still seems to be a defvar
in this diff. I don't see why this would be a function.
I suspect it's meant to grab either the regular buffer or the socket buffer. If that's so, what do you do if both are set? Not sure if the regular buffer is cleared when you start a socket repl. But if you choose a different repl with inf-clojure-set-repl
, can you swap back to a socket repl? Seems like nothing knows to set the socket repl buffer except for the code to actually start the process.
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.
the regular buffer or the socket buffer.
Socket buffer as in the server process buffer? (the REPL buffer is the connection process already)
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.
inf-clojure-buffer
shouldn't be invoked, this is leftover from when I had a function to figure out which buffer to send evaluations to dependent on repl-type
- this is just inf-clojure-buffer
accessed as the original variable, thanks!
Btw, this PR also prompted me to finally update the CI setup and add some basic linting to it. Please, rebase on top of master so this will run automatically for your PR. |
Yeah, I think this is a REPL that you need to "upgrade" manually afterwards. |
That's how I was using it but as a default I am now using is this Also implicit use of clojure.main with options is deprecated so I've updated the clojure/cljs bindings with an additional I haven't actually pushed yet, still need to rebase and run the tests but I've made the changes locally - will update once this is done. Thanks guys for your reviews/feedback so far it's much appreaciated! |
1ab3e93
to
b37b28d
Compare
Alright I've rebased from clojure-emacs/master and pushed changes that I think addresses your feedback @bbatsov & @dpsutton . I've also run
Should we try and fix these here too? I've left these as I'm not sure of the implications |
We can handle those separately. |
Is there anything that else I can do to help here? I can't seem to see the CircleCI pipeline for this project (though I can see cider's) so I'm can't tell if they've passed there. However I have run the tests locally inside emacs with buttercup-run-at-point and that seems to work. I guess I should squash these commits? |
@mikepjb this is really cool. |
It does seem pretty awesome! The downside here is that this invocation requires clojurescript so running it outside a project by default results in an error. Otherwise it seems like a good idea. As a side note there's also a Tested using: Edit: I did also test this with inline |
inf-clojure.el
Outdated
(cljs . "clojure -J-Dclojure.server.repl=\"{:port %s :accept cljs.server.browser/repl}\"") | ||
(lein-clr . "JVM_OPTS='-Dclojure.server.repl={:port %s :accept clojure.core.server/repl}' lein clr repl") | ||
(planck . "planck -n %s") | ||
(babashka . "bb socket-repl %s"))) |
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.
nbb seems to be missing from this 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.
I've had a look at this and while the startup form is simple nbb socket-repl :port %s
, the feature forms are not the same as bb
so this seems like a significant piece of work (at least for me, having only just learned about nbb
now!)
Things like (clojure.core/load-file "%s")
aren't available, I had a quick look at using node alternatives e.g fs/readFileSync
but couldn't get it working inline.
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.
Got it. I guess we can skip it for now then.
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.
inf-clojure.el
Outdated
nil | ||
"Port to be used when creating a socket REPL via `inf-clojure-socket-repl'. | ||
If left as nil a random port will be selected between 5500-6000." | ||
:type '(choice integer (const nil))) |
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 should also have a :package-version
property.
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.
added this as version 3.2.1
inf-clojure.el
Outdated
`inf-clojure-custom-startup' if this is defined." | ||
(interactive (list (or (unless current-prefix-arg | ||
inf-clojure-custom-startup) | ||
(completing-read "Select Clojure REPL startup command: " |
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.
maybe "Clojure socket REPL command"?
inf-clojure.el
Outdated
(setq inf-clojure-socket-repl-type | ||
repl-type | ||
inf-clojure-custom-repl-name | ||
;; comint mode includes the ** chars |
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 don't get this comment.
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's a reminder that supplying a name to comint e.g my-awesome-repl
actually results in the name *my-awesome-repl*
but I will remove this comment
inf-clojure.el
Outdated
(socket-form (or (cdr (assoc repl-type inf-clojure-socket-repl-startup-forms)) | ||
inf-clojure-custom-startup | ||
cmd)) | ||
(socket-cmd (format socket-form (number-to-string port))) |
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.
You can just use a %d
format specifier, so you don't need to convert the number to string like this.
inf-clojure.el
Outdated
inf-clojure-custom-startup | ||
cmd)) | ||
(socket-cmd (format socket-form (number-to-string port))) | ||
(sock (let ((default-directory (or (clojure-project-dir) default-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.
I've noticed this is the second clojure-project-dir
call in the big let
, so you might want to factor it out as a let
binding.
inf-clojure.el
Outdated
(when inf-clojure-socket-callback | ||
(funcall inf-clojure-socket-callback))))))) | ||
|
||
(defun inf-clojure-repl-sentinel (process event) |
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.
socket-repl-sentinel
CHANGELOG.md
Outdated
@@ -4,6 +4,7 @@ | |||
* [#202](https://github.com/clojure-emacs/inf-clojure/issues/202): Add ClojureCLR support. | |||
* [#204](https://github.com/clojure-emacs/inf-clojure/issues/204): Scroll repl buffer on insert commands | |||
* [#208](https://github.com/clojure-emacs/inf-clojure/pull/208) Display message after setting repl. | |||
* [#210](https://github.com/clojure-emacs/inf-clojure/pull/210) Include `inf-clojure-socket` to create a socket REPL and connect to it from inside Emacs. |
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.
socket-repl
I've added a few extra remarks. Feel free to squash after addressing them and I think we're good to go. |
746dd5d
to
6824d07
Compare
All done and squashed now, the only thing I'm not too sure about is the inclusion of |
inf-clojure.el
Outdated
"Port to be used when creating a socket REPL via `inf-clojure-socket-repl'. | ||
If left as nil a random port will be selected between 5500-6000." | ||
:type '(choice integer (const nil)) | ||
:package-version '(inf-clojure . "3.2.1")) |
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 should be "3.3" - the version in which it will be shipped.
6824d07
to
8d15233
Compare
Ok updated again with 3.3 as the package-version for |
8d15233
to
b74e9d1
Compare
I've just pushed a fix for a bug I found where supplying your own
|
inf-clojure-socket is a helper function to create and connect to a socket REPL from within Emacs. [clojure-emacs#209] pass :repl-type to inf-clojure-connect [clojure-emacs#209] Automatic REPL selection by type When multiple REPLs are running, it is hardly useful to evaluate Clojurescript inside a Clojure REPL so there are two global inf-clojure buffers now for each 'sub'-language cljs & clj. When running inf-clojure functions, we take into account the major-mode before deciding which REPL should perform evaluation. [clojure-emacs#209] Correct changelog PR link [clojure-emacs#209] fix modeline + better socket form support [clojure-emacs#209] close socket buffer when REPL buffer closed The user gets asked if they want to close each buffer sequentially. Or whatever their process-kill-query-function dictates. [clojure-emacs#209] ensure space for clojure cli args [clojure-emacs#209] remove cljs/dual buffer & keep this PR simple [clojure-emacs#209] clean up! [clojure-emacs#209] implement PR feedback More simplification, more consistent naming conventions and also cleaning up bugs. [clojure-emacs#209] linting fixes [clojure-emacs#209] bug fix with manually set socket-repl cmd [clojure-emacs#209] include node-babashka and fix PR remarks
b74e9d1
to
e27b989
Compare
and another issue that I've pushed a fix for, |
This PR addresses the issue #209 I raised yesterday, introducing the
inf-clojure-socket
function to create and connect to a socket REPL from within Emacs.Additionally:
inf-clojure-buffer
as before and alsoinf-clojure-cljs-buffer
. This is a somewhat dirty solution that builds on the current implementation, this is useful as it's common for projects to use clj & cljs together.inf-clojure--get-buffer
returns the right global buffer variableinf-clojure-buffer
orinf-clojure-cljs-buffer
depending on whether your are currently in a clojurescript buffer/file.inf-clojure-cljs-buffer
to avoid this PR ballooning, I've only done the neccessary changes to allow clojurescript-mode buffers to eval code seperately. Functions likeinf-clojure-set-repl
,inf-clojure-clear-repl-buffer
andinf-clojure-connected-p
have been left asinf-clojure-buffer
only functions for example.I've tested this using:
M-x inf-clojure-socket
M-: (inf-clojure-socket :port 5555 :repl-type "clojure")
M-: (inf-clojure-socket :repl-type "cljs")
As well as setting
inf-clojure-cli-args
to nil and "-Mdev" to make sure this was being included when the socket server was started.Should we include keybinds for launching clj, cljs and clj&cljs socket REPLs like cider jack-in?
Unsure about naming conventions but I've made my best guesses, so any recommendations for changes please let me know!