-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
Show startup commands in repl buffer #2744
Conversation
Nicely done! I think, however, it's a bit misleading that the shell command that boots nREPL and the Clojure form that changes a REPL type are shown in the same way, because they clearly aren't. It also seems to me that this will work only for REPLs which came from cider-jack-in, so some command to describe a REPL might be useful as well. And perhaps we should show the ClojureScript version in use somewhere. Somehow I never thought about this. |
I was envisioning making sure it was clear how a repl is started. If someone is not using jack-in commands then it is already clear to them how it was started.
I don't follow this. I'm showing the clojurescript startup command. Or do you mean the cljs repl type? Like shadow or figwheel etc? |
I was thinking of:
Or something along those lines. Similar to the info you get on top of the REPL. I guess it's easy to infer what's the flavour based on the form, but it might look nicer for the users.
Fair point. I guess the ClojureScript info, however, can always be displayed as we always upgrade a Clojure REPL and you can't really connect to a ClojureScript session. |
I've added the cljs repl type and done a bit of formatting so that its grouped. The only thing that is missing would be perhaps the clojurescript version and i guess the node version if running on node. I think these present a pretty annoying problem.
|
I guess we can have either the |
@dpsutton One more thing - I notice that something's weird with the REPL prompt in your example. First there's the |
@@ -333,6 +334,28 @@ fully initialized." | |||
(insert-before-markers | |||
(propertize (cider-repl--help-banner) 'font-lock-face 'font-lock-comment-face)))) | |||
|
|||
(defun cider-repl--insert-startup-commands () |
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 thought a bit more about this and I'd prefer to split this in two - the jack-in command that gets displayed in the REPL banner if present and the ClojureScript details which can be placed/displayed as you've suggested. I just feel that the jack-in command is more fundamental than the ClojureScript form, that's why it should be listed more prominently. Potentially we can add there some info about the build system down the road - e.g. Leiningen 2.9, etc.
@dpsutton ping :-) |
6a6bdf2
to
8aa19ff
Compare
Yeah, something like this. We just need some fallback for the jack-in command in case it's not present. Probably we can just move it to the very top and prepend it to the rest of the banner if needed. |
cider-repl.el
Outdated
@@ -333,6 +335,26 @@ fully initialized." | |||
(insert-before-markers | |||
(propertize (cider-repl--help-banner) 'font-lock-face 'font-lock-comment-face)))) | |||
|
|||
(defun cider-repl--insert-param-values (param-tuples) |
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.
We don't call this often, so I'd go with explicit params here. I think it's going to read much nicer.
8aa19ff
to
92da68c
Compare
@bbatsov this should be what you are looking for if i understand you correctly. The startup command is now in the banner along with the other info and the cljs stuff is outside and grouped together |
CHANGELOG.md
Outdated
* [#2499](https://github.com/clojure-emacs/cider/issues/2499): Add `cider-jump-to-pop-to-buffer-actions`. | ||
* Show clj and cljs startup commands in repl 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've duplicated this in the changelog.
test/cider-repl-tests.el
Outdated
@@ -38,7 +70,8 @@ | |||
(spy-on 'cider--nrepl-version :and-return-value "0.5.3") | |||
(setq nrepl-endpoint (list :host "localhost" :port "54018")) | |||
(setq cider-version "0.12.0") | |||
(setq cider-codename "Seattle")) | |||
(setq cider-codename "Seattle") | |||
(setq cider-saved-params (list :jack-in-cmd "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.
I think you're not using this.
cider-connection.el
Outdated
@@ -699,6 +699,7 @@ PARAMS is a plist as received by `cider-repl-create'." | |||
(declare-function cider-repl-reset-markers "cider-repl") | |||
(defvar-local cider-session-name nil) | |||
(defvar-local cider-repl-init-function nil) | |||
(defvar-local cider-saved-params 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'd name this cider-launch-params
or something like this.
cider-repl.el
Outdated
(when (or cljs-repl-type cljs-init-form) | ||
(emit-comment "") | ||
(when cljs-repl-type | ||
(emit-comment (concat "cljs repl type: " (symbol-name cljs-repl-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.
Let's use ClojureScript REPL type:
here.
cider-repl.el
Outdated
(when cljs-repl-type | ||
(emit-comment (concat "cljs repl type: " (symbol-name cljs-repl-type)))) | ||
(when cljs-init-form | ||
(emit-comment (concat "cljs repl startup command: " cljs-init-form))) |
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.
ClojureScript REPL init form:
@dpsutton Looks good. I've added a couple of tiny cosmetic remarks and we're good to go. |
1c63258
to
548be3d
Compare
Thanks for working on this! 🙇 |
Thanks for the thorough checking of the code! much appreciated |
Simple change to show the commands CIDER uses to get everything running to the user in the repl buffer.
There are a few good reasons to do this:
we show the jack in command but its as a message and can quickly go away
we never show the cljs command so people might forget how they are actually starting the cljs repl when CIDER doesn't display it
it removes the "magic" of what CIDER does for beginners and trouble shooters.
it gives someone access to the commands if they prefer to
cider-connect
instead of having clojure running as a subprocess to emacs.The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (
make test
)All code passes the linter (
make lint
) which is based onelisp-lint
and includescheckdoc
, check-declare, packaging metadata, indentation, and trailing whitespace checks.You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)
Thanks!
If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.