Skip to content

Support cider.clj-reload/reload ops #3624

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+54 −4
Closed

Support cider.clj-reload/reload ops #3624

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23b1445
Support cider.clj-reload/reload ops
filipesilva Mar 2, 2024
d832797
review items
filipesilva Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -7,6 +7,9 @@
- [#3622](https://github.com/clojure-emacs/cider/pull/3461): Basic support for using CIDER from [clojure-ts-mode](https://github.com/clojure-emacs/clojure-ts-mode).
- The `clojure-mode` dependency is still required for CIDER to function.
- Some features like `cider-dynamic-indentation` and `cider-font-lock-dynamically` do not work with `clojure-ts-mode` (yet).
- [#3624](https://github.com/clojure-emacs/cider/pull/3624): Support cider.clj-reload/reload ops
- adds `cider-ns-refresh-tool` defcustom, defaulting to `'tools.namespace`
- you can change it to `'clj-reload` to use [clj-reload](https://github.com/tonsky/clj-reload) instead of [tools.namespace](https://github.com/clojure/tools.namespace).

### Changes

33 changes: 29 additions & 4 deletions cider-ns.el
View file
Original file line number Diff line number Diff line change
@@ -118,6 +118,14 @@ namespace-qualified function of zero arity."
:group 'cider
:package-version '(cider . "0.10.0"))

(defcustom cider-ns-code-reload-tool 'tools.namespace
"Which tool to use for ns refresh.
Current options: tools.namespace and clj-reload."
:group 'cider
:type '(choice (const :tag "tools.namespace https://github.com/clojure/tools.namespace" tools.namespace)
(const :tag "clj-reload https://github.com/tonsky/clj-reload" clj-reload))
:package-version '(cider . "1.14.0"))

(defun cider-ns--present-error (error)
"Render the `ERROR' stacktrace,
and jump to the adequate file/line location,
@@ -156,7 +164,7 @@ presenting the error message as an overlay."

(defun cider-ns-refresh--handle-response (response log-buffer)
"Refresh LOG-BUFFER with RESPONSE."
(nrepl-dbind-response response (out err reloading status error error-ns after before)
(nrepl-dbind-response response (out err reloading progress status error error-ns after before)
(cl-flet* ((log (message &optional face)
(cider-emit-into-popup-buffer log-buffer message face t))

@@ -184,6 +192,9 @@ presenting the error message as an overlay."
(reloading
(log-echo (format "Reloading %s\n" reloading) 'font-lock-string-face))

(progress
(log-echo progress 'font-lock-string-face))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The progress key is a passthrough of the logging cli-reload outputs. It doesn't give you a list of files being reloaded per se, just logs as they are individually unloaded/reloaded. Examples:

cider-ns-refresh: Nothing to reload
cider-ns-refresh: Reloading successful

cider-ns-refresh: Unloading scratch.scratch
cider-ns-refresh: Loading scratch.scratch
cider-ns-refresh: Reloading successful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems nice. However for large projects it might get excessively noisy?

I'd suggest a defcustom, default "no logging" (since one isn't debugging stuff by default)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how this can get very spammy if you're looking at the *messages* buffer, and agree it should be good to prevent that spam.

I worry preventing it might be premature though... it is adding another customisation, and I imagine this feature might not have a lot of users for a while. Perhaps it could prove unnecessary.

I defer to your opinion though. I don't look at *messages* a lot. Maybe spam there is a big issue.

Copy link
Contributor Author

@filipesilva filipesilva Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRT the default of no logging, there's some tension there. Reloading, especially on larger projects, can take a while. Seeing the progress is helpful, and tools.namespace not showing the progress is actually to its detriment.

So larger projects, which are the ones where the progress report would be more spammy, are also the ones that care more about the current progress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CIDER generally uses spinner for progress indication needs.

(it's an item that popped up in a recent PR)

It could be added at some point. Please do not feel pressured to do so, but it's what I'd expect before the "spammy" default :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spinner would be quite easy to add indeed. I agree that the extra logging should probably be optional (and maybe displayed in a standalone buffer one can quickly discard). Not a big deal at this stage, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, there's already cider-ns-refresh-show-log-buffer, which I guess it makes sure to reuse if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And because we're already using log-echo it seems we don't have to worry much about spamming Messages here:

(log-echo (message &optional face)
                         (log message face)
                         (unless cider-ns-refresh-show-log-buffer
                           (let ((message-truncate-lines t))
                             (message "cider-ns-refresh: %s" message))))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I'm not sure what the end result is here. Is it that I should add a spinner, or hat I shouldn't do anything since cider-ns-refresh-show-log-buffer already exists?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me you don't need to do anything, as those logs will end in the dedicated buffer (when enabled).

We can discuss the need for adding a spinner down the road.


((member "reloading" (nrepl-dict-keys response))
(log-echo "Nothing to reload\n" 'font-lock-string-face))

@@ -223,6 +234,19 @@ Its behavior is controlled by `cider-ns-save-files-on-refresh' and
(file-in-directory-p buffer-file-name dir))
dirs)))))))

(defun cider-ns--reload-op (op-name)
"Return the reload operation to use.
Based on OP-NAME and the value of cider-ns-code-reload-tool defcustom."
(list "op"
(if (eq cider-ns-code-reload-tool 'tools.namespace)
(cond ((string= op-name "reload") "refresh")
((string= op-name "reload-all") "refresh-all")
((string= op-name "reload-clear") "refresh-clear"))

(cond ((string= op-name "reload") "cider.clj-reload/reload")
((string= op-name "reload-all") "cider.clj-reload/reload-all")
((string= op-name "reload-clear") "cider.clj-reload/reload-clear")))))

;;;###autoload
(defun cider-ns-reload (&optional prompt)
"Send a (require 'ns :reload) to the REPL.
@@ -275,9 +299,10 @@ refresh functions (defined in `cider-ns-refresh-before-fn' and
(interactive "p")
(cider-ensure-connected)
(cider-ensure-op-supported "refresh")
(cider-ensure-op-supported "cider.clj-reload/reload")
(cider-ns-refresh--save-modified-buffers)
(let ((clear? (member mode '(clear 16)))
(refresh-all? (member mode '(refresh-all 4)))
(all? (member mode '(refresh-all 4)))
(inhibit-refresh-fns (member mode '(inhibit-fns -1))))
(cider-map-repls :clj
(lambda (conn)
@@ -292,11 +317,11 @@ refresh functions (defined in `cider-ns-refresh-before-fn' and
nil
t))
(when clear?
(cider-nrepl-send-sync-request '("op" "refresh-clear") conn))
(cider-nrepl-send-sync-request (cider-ns--reload-op "reload-clear") conn))
(cider-nrepl-send-request
(thread-last
(map-merge 'list
`(("op" ,(if refresh-all? "refresh-all" "refresh")))
`(,(cider-ns--reload-op (if all? "reload-all" "reload")))
(cider--nrepl-print-request-map fill-column)
(when (and (not inhibit-refresh-fns) cider-ns-refresh-before-fn)
`(("before" ,cider-ns-refresh-before-fn)))
22 changes: 22 additions & 0 deletions doc/modules/ROOT/pages/usage/misc_features.adoc
View file
Original file line number Diff line number Diff line change
@@ -130,6 +130,28 @@ and `cider-ns-reload-all` commands can be used instead. These commands
invoke Clojure's `+(require ... :reload)+` and `+(require
... :reload-all)+` commands at the REPL.

You can also use https://github.com/tonsky/clj-reload[clj-reload] instead.
It provides supports
https://github.com/tonsky/clj-reload?tab=readme-ov-file#usage-keeping-vars-between-reloads[keeping vars between reloads]
among some
https://github.com/tonsky/clj-reload?tab=readme-ov-file#comparison-toolsnamespace[other differences]
from `tools.namespace`.

[source,lisp]
----
(setq cider-ns-code-reload-tool 'clj-reload)
----

With `clj-reload` you should set the source dirs as described in
https://github.com/tonsky/clj-reload?tab=readme-ov-file#usage[the usage docs]
. If you don't set them manually, it will default to the current source dirs in the same
way `tools.namespace` does.

Another difference is that `cider-ns-refresh-before-fn` and `cider-ns-refresh-after-fn` are
not used with `clj-reload`, but in turns it supports per-namespace
https://github.com/tonsky/clj-reload?tab=readme-ov-file#usage-unload-hooks[unload hooks]
.

== CIDER Selector

The `cider-selector` (kbd:[C-c M-s]) command allows you to quickly navigate to