Display message after setting repl #208
Conversation
inf-clojure.el
Outdated
| (when-let ((repl-buffer (completing-read "Select default REPL: " repl-buffers nil t))) | ||
| (setq inf-clojure-buffer (get-buffer repl-buffer))) | ||
| (user-error "No buffers have an inf-clojure process")))) | ||
| (message "inf-clojure repl set to %s" inf-clojure-buffer)) |
There was a problem hiding this comment.
Probably it'd be better to use something like "Current/active inf-clojure REPL set to ...".
There was a problem hiding this comment.
Thanks for the input, I made the change!
|
thanks for the addition. Great usability addition. Once you update to @bbatsov 's suggestion for the message we'll merge. |
jjttjj
force-pushed
the
set-repl-message
branch
from
e00b48b to
5e652d7
Compare
CHANGELOG.md
Outdated
| @@ -3,6 +3,7 @@ | |||
| ## master (unreleased) | |||
| * [#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 | |||
| * Display message after setting repl. | |||
There was a problem hiding this comment.
you should add a link here
- #208 Display message after setting repl with
inf-clojure-set-repl
inf-clojure.el
Outdated
| (when-let ((repl-buffer (completing-read "Select default REPL: " repl-buffers nil t))) | ||
| (setq inf-clojure-buffer (get-buffer repl-buffer))) | ||
| (user-error "No buffers have an inf-clojure process")))) | ||
| (message "Current inf-clojure REPL set to %s" inf-clojure-buffer)) |
There was a problem hiding this comment.
Also, should this message move this to be inside of the when-let which sets inf-clojure-buffer?
There was a problem hiding this comment.
You're right, I missed that possible outcome.
I changed it to only display the message after a successful change. Not confident in my style choices here, I could also just repeat the call to message in both success branches separately, let me know if you have a preference.
https://github.com/jjttjj/inf-clojure/blob/04b967f1e60adb79cd84d580268f7af00021eb71/inf-clojure.el#L284-L294
There was a problem hiding this comment.
I don't have any strong opinions. I lean towards having the message and the setq in the same body so they work in tandem. But it doesn't really matter.
jjttjj
force-pushed
the
set-repl-message
branch
from
5e652d7 to
04b967f
Compare
inf-clojure.el
Outdated
| (user-error "No buffers have an inf-clojure process")))))) | ||
| (when new-repl-buffer | ||
| (message "Current inf-clojure REPL set to %s" new-repl-buffer)))) | ||
| (if (and (not always-ask) |
There was a problem hiding this comment.
The nest ifs make it a bit hard to follow the code, so some comments at the beginning of each branch might be useful.
inf-clojure.el
Outdated
| (inf-clojure-repl-p)) | ||
| (progn | ||
| (setq inf-clojure-buffer (current-buffer)) | ||
| (message "Current inf-clojure REPL set to %s" inf-clojure-buffer)) |
There was a problem hiding this comment.
To reduce this message duplication you can just return the new-repl buffer at the end and set it outside the ifs with a single message there. You can even the the code looking for the new repl to be a separate private function, so this would read a bit better. I'd prefer such less imperative code structure.
|
After trying a few arrangements, I went with extracting a function to just handle the "select repl" prompt. I believe this could also be reused to refactor this if you would like. |
Yeah, it'd be nice to remove the code duplication, so a follow-up PR will be appreciated.
No worries! The code is better in the end and that is all that matters. |
Displays a message after setting the repl.
When manually setting a repl with
inf-clojure-set-repl, there is no visual indication that the repl has successfully been changed. This has confused me a few times in the past so I added a message.This was briefly discussed on slack
Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
M-x checkdocwarningsThanks!