Add cider-reload-ns and cider-ns-reload-all interactive command #2314

arichiardi · 2018-06-08T16:46:14Z

In order to send (require 'ns :reload/:reload-all) more easily.

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our [contribution guidelines][1]
You've updated the [changelog][3] (if adding/changing user-visible functionality)
You've updated the [user manual][4] (if adding/changing user-visible functionality)

vspinu · 2018-06-08T21:09:16Z

I think cider-eval-ns-form could do this instead of adding a new command.

arichiardi · 2018-06-08T21:17:16Z

I would be fine with this, however however this command can also ask if you want to reload something explicitly. Very handy when you don't need to evaluate the whole ns form but only part of it.

We could add this as well to cider-eval-ns-form actually...so...well I am open to both approaches.

bbatsov · 2018-06-09T07:32:31Z

cider-interaction.el

+(defun cider-reload (arg)
+  "Send a require :reload/:reload-all to the REPL.
+
+The prefix argument ARG can change the behavior of the command:


I'd be useful to explain here why would users want to prefer this over cider-refresh.

arichiardi · 2018-06-09T21:55:20Z

@bbatsov what is you opinion on using cider-eval-ns-form for this?

bbatsov · 2018-06-10T12:25:05Z

I'm fine with this idea.

arichiardi · 2018-06-12T23:51:20Z

cider-interaction.el

+
+The prefix argument ARG can change the behavior of the command:
+
+  - (4)   \\[cider-reload]: prompts for a namespace name.


If there is a better way to show this, without havin a C-u embedded in docstring warning, I am eager to know.

There must be - as I can't understand your notation at all. :D

Seems to me that probably 3 prefix arguments are a bit too much. Perhaps you don't really need to be able to operate on other namespaces and you could just have one prefix arg for reload-all?

Actually maybe you are right and we should tune it for the most common cases - say I am working on ns A and I need to change it B:

Flow 1)

open ns B

modify

save

reload the I am in - B

Flow 2)

open B

modify

save

go to A

reload all in A

The ability of inputting a namespace stems from the fact that I am not a master of reload-all sometimes and I need to reload ad hoc a namespace that threw an error (was still stale), handy because I do need to open its buffer.

arichiardi · 2018-06-13T00:49:57Z

cider-mode.el

@@ -273,6 +273,8 @@ Configure `cider-cljs-repl-types' to change the ClojureScript REPL to use for yo
     ["Clear latest output" cider-find-and-clear-repl-output]
     ["Clear all output" (cider-find-and-clear-repl-output t)
      :keys "\\[universal-argument] \\[cider-find-and-clear-repl-output]"]
+     ["Require and reload ns" (cider-reload t)
+      :keys "\\[universal-argument] \\[cider-reload]"]


Also need some advice on this one.

bbatsov · 2018-06-17T20:27:55Z

cider-interaction.el

+  :reload forces loading of all the identified libs even if they are already
+loaded
+  :reload-all implies :reload and also forces loading of all libs that the
+identified libs directly or indirectly load via require [...]"


This should definitely be mentioned, though - https://github.com/clojure/tools.namespace#reloading-code-motivation

Do you mean mention tools.namespace as alternative to reload?

bbatsov · 2018-06-17T20:28:41Z

cider-mode.el

@@ -273,6 +273,8 @@ Configure `cider-cljs-repl-types' to change the ClojureScript REPL to use for yo
     ["Clear latest output" cider-find-and-clear-repl-output]
     ["Clear all output" (cider-find-and-clear-repl-output t)
      :keys "\\[universal-argument] \\[cider-find-and-clear-repl-output]"]
+     ["Require and reload ns" (cider-reload t)


Why did you put this here?

I saw the other pattern, should it go somewhere else? Or take it away completely?

I meant that the local of this is kind of arbitrary.

Sorry I don't really get what this should be instead, can you dumb it down a bit 😄

Local -> location. I meant that it should be somewhere around cider-refresh.

Oh ok cool I will move it there no problem

vspinu · 2018-06-18T10:28:09Z

BTW, I think cider-reload is not specific enough. It might be confused with cider-restart which does very different things.

arichiardi · 2018-06-18T15:28:03Z

Clojure "reload" users should know the difference between the two I guess. There is not key binding for it at the moment so if you opt in probably you know the difference?

bbatsov · 2018-06-21T20:56:44Z

Both of you make valid points - the current names can probably be improved, although generally most people should know what to do. Naming is hard! Maybe cider-reload-ns/buffer might be a better name. I don't have any great ideas right now.

arichiardi · 2018-07-01T00:43:26Z

@bbatsov where should this go now? cider-eval.el? It is eval-ing something... cider-refresh.el?

bbatsov · 2018-07-01T08:54:20Z

I think it can go to cider-common.el if we plan to use this from repls, or to cider-mode.el otherwise.

arichiardi · 2018-07-01T14:46:15Z

Ok cool, yes it is a repl command, actually not unless there is way to find the current namespace at the repl..probably there is

arichiardi · 2018-07-01T15:55:04Z

@bbatsov @vspinu I was thinking, what if the reload I have can be selected with a defcustom?

There are cases also when the cider-refresh would not work, one could be the absence of the middleware, the other is ClojureScript. Folks might want to disable it completely and use the (require ... :reload) instead.

arichiardi · 2018-07-01T15:58:14Z

Well no, the functions have different arguments, completely different.

bbatsov · 2018-07-01T21:28:10Z

cider-mode.el

@@ -416,6 +418,7 @@ If invoked with a prefix ARG eval the expression after inserting it."
 (declare-function cider-toggle-trace-ns "cider-tracing")
 (declare-function cider-toggle-trace-var "cider-tracing")
 (declare-function cider-refresh "cider-refresh")
+(declare-function cider-reload-ns "cider-reload-ns")


That's not exactly correct. ;-)

bbatsov · 2018-07-01T21:28:38Z

cider-refresh.el

@@ -141,6 +171,37 @@ Its behavior is controlled by `cider-save-files-on-cider-refresh'."
                           (file-truename default-directory)
                           (eq system-type 'windows-nt))))))))

+;;;###autoload
+(defun cider-reload-ns (&optional arg)


Reload is different from refresh, so I wouldn't keep in the same source file.

Uhm why not? the two actually do the same thing. cider-refresh does it a bit better because of clojure.tools.namespace. I put the explanation in the commentary and it is evident from there that the end result is the same. That is why I opted for putting in here. If you feel very strong about moving, of course we will move it, please pick a file ;)

To be frank I am still not very clear about the difference. From the commentary which you added looks like refresh is uniformly better, right? Then why to bother with ns level reload? Does it have specific advantages besides small improvements in speed?

It is not speed the advantage, it is not using tools.namespace for something simple which is provided by Clojure natively. No middleware involved - less potential breakage

@arichiardi Noticed that everything in this file is about cider-refresh. Notice also that every function here has the cider-refresh prefix. cider-reload-ns simply looks out of place here. That's why I suggest cider-common.el or just putting this in cider-mode.el if you don't envision it to be used from the REPL much.

Oh ok yes that makes sense, unless we call it cider-refresh-native-reload

Or cider-refresh-require-reload

I actually have a better idea finally. We rename cider-refresh.el to cider-ns.el and we put there cider-refresh as cider-ns-refresh and cider-reload-ns, as cider-ns-reload. This will also make it a natural home for things like cider-ns-eval and cider-ns-cleanup, etc.

Oh I like this will do it

vspinu · 2018-07-02T08:03:35Z

cider-refresh.el

+;; http://thinkrelevance.com/blog/2013/06/04/clojure-workflow-reloaded
+;;
+;; Note that refresh with clojure.tools.namespace.repl is a smarter way to
+;; reload code.


If I understand correctly, here you mean that refresh is "smarter" compared to "traditional reload" which you describe below? Wouldn't it be clearer to first describe "traditional reload" and then list all those features below as "improvements" of the refresh and not "problems" of reload?

I think this is a good write up and could be even placed in the manual.

I can do that - however I am not writing this myself but it is in the README of tools.namespace that Bohzidar linked. So kudos to the original writer.

bbatsov · 2018-07-02T17:39:31Z

cider-refresh.el

+         (form (concat "(require '%s " (if (not reload-all-p)
+                                           ":reload)"
+                                         ":reload-all)"))))
+    (cider-interactive-eval (format form ns))))


Btw, with the default handler users won't really get any specific feedback the ns being reloaded. They'll just see some form got evaluated.

Uhm that is not good, there should be at least the spinner going. Which handler would you recommend?

Leave this for now. I'll think of some more versatile handler later. Basically we just need something like the current handler that should be aware if it was invoked in a REPL and then print its out there.

See #2314 for the rationale behind this change. Basically we plan to put several ns manipulation commands together in the same module.

arichiardi · 2018-07-22T20:23:15Z

Oh ok didn't notice that you've already renamed. I will need to do a cleanup and rebase.

bbatsov · 2018-07-23T06:53:48Z

👍

arichiardi · 2018-08-01T16:47:27Z

Revisited this, splitting in two commands and assigning them to C-c M-n l and C-c M-n M-l respectively.

bbatsov · 2018-08-02T05:35:51Z

cider-ns.el

+         (ns (if prompt-p
+                 (string-remove-prefix "'" (read-from-minibuffer "Namespace: " (clojure-find-ns)))
+               (clojure-find-ns)))
+         (form (concat "(require '%s :reload)")))


What's this concat here for? :-) I think you could have used format here directly to add the ns instead of doing it a line done.

Yep sorry will fix that, leftover from the previous impl

bbatsov · 2018-08-02T05:36:31Z

cider-ns.el

+         (ns (if prompt-p
+                 (string-remove-prefix "'" (read-from-minibuffer "Namespace: " (clojure-find-ns)))
+               (clojure-find-ns)))
+         (form (concat "(require '%s :reload-all)")))


vspinu · 2018-08-02T07:58:41Z

cider-ns.el

+(defun cider-ns-reload (&optional arg)
+  "Send a (require current-ns :reload) to the REPL.
+
+With the universal prefix ARG, prompts for a namespace name and then


"and then" what? :)

Lol thanks, again did some poor job in splitting the original fn + comment yesterday

vspinu · 2018-08-02T07:59:50Z

cider-ns.el

+on org.clojure/tools.namespace.  See Commentary of this file for a longer
+list of differences.  From the Clojure doc:
+
+  :reload forces loading of all the identified libs even if they are


Would it be possible to avoid empty lines in the docs unless strictly necessary? Here you could simple quote like this: From the Clojure doc ":reload forces ...".

vspinu · 2018-08-02T08:01:36Z

cider-ns.el

+  :reload forces loading of all the identified libs even if they are
+already loaded"
+  (interactive "P")
+  (let* ((prompt-p (or (equal arg '(4)) (equal arg '(-4))))


arg has no other function right? So you could name your argument prompt-p directly. Alos -p prefix is reserved for functions (predicates). We use prompt and do-prompt elsewhere.

Good catch. I guess this is a leftover from the original implementation that had a different behavior for single and double prefix argument. I really dislike double prefix args, so I asked @arichiardi to split this in two commands.

Cool will do

vspinu · 2018-08-02T08:03:03Z

cider-ns.el

+;; Smart code refresh functionality based on ideas from:
+;; http://thinkrelevance.com/blog/2013/06/04/clojure-workflow-reloaded
+;;
+;; Note that refresh with clojure.tools.namespace.repl is a smarter way to


Smarter compared to what? Now that this file contains loads of other ns functionality this comment should be more explicit regarding what it refers to.

merged with the following line down there

vspinu · 2018-08-02T08:05:53Z

cider-ns.el

+(defun cider-ns-reload-all (&optional arg)
+  "Send a (require current-ns :reload-all) to the REPL.
+
+With the universal prefix ARG, prompts for a namespace name and then


vspinu · 2018-08-02T08:12:44Z

Are you sure about the keys? Usually base keys like l and M-l serve for the same command. In my experience when two similar commands are hooked on similar keys it's very easy to confuse them. But maybe it's not a problem in this case because one is a super-set of the other, right?

An alternative would be to have one command proposing *all* option along other namespaces when used with the prefix argument. *all* could be the default such that C-u C-c l RET would get us the :reload-all.

bbatsov · 2018-08-02T11:48:14Z

I think it's fine in this this case, although probably we can have M-l and l for reload and M-a and a for reload-all. I also comtemplated on whether to comment on this in light of how most of CIDER behaves, but I think l and M-l would be ok as well, as nothing really bad can come out from mixing those up.

arichiardi · 2018-08-02T14:37:13Z

Btw why do we reserve two keys per command? In this case it felt better to change the pattern a bit but I am open to any solution you guys find better

The C-c M-n l has been assigned to cider-ns-reload while C-c M-n M-l has been assigned to cider-ns-reload-all.

bbatsov · 2018-08-02T18:09:24Z

Common Emacs convention, as some people (like me) prefer not to lift their finger holding down some modifier key, and some don't. :-)

arichiardi · 2018-08-02T18:44:21Z

Oh got it, I think now understand better your comment above about the behavior and yes, the worst you observe if you :reload-all instead of :reload is that it takes more time. Of course this method is not really optimal (that is why the tools.namespace method exists) so most of the times I personally do not rely on the exactness of it.

bbatsov reviewed Jun 9, 2018

View reviewed changes

arichiardi force-pushed the require-reload branch 2 times, most recently from da2c2fc to 6890f13 Compare June 12, 2018 23:42

arichiardi commented Jun 12, 2018

View reviewed changes

arichiardi force-pushed the require-reload branch 2 times, most recently from d0e8642 to ee87cb0 Compare June 13, 2018 00:12

arichiardi commented Jun 13, 2018

View reviewed changes

bbatsov reviewed Jun 17, 2018

View reviewed changes

arichiardi force-pushed the require-reload branch 2 times, most recently from f26efb5 to 3f6a45e Compare July 1, 2018 16:50

arichiardi changed the title ~~Add cider-reload interactive command~~ Add cider-reload-ns interactive command Jul 1, 2018

arichiardi force-pushed the require-reload branch from 3f6a45e to 3c918a4 Compare July 1, 2018 16:52

bbatsov reviewed Jul 1, 2018

View reviewed changes

arichiardi force-pushed the require-reload branch 3 times, most recently from 9036792 to 316da57 Compare July 1, 2018 21:59

vspinu reviewed Jul 2, 2018

View reviewed changes

bbatsov reviewed Jul 2, 2018

View reviewed changes

bbatsov added a commit that referenced this pull request Jul 10, 2018

Rename the cider-refresh.el to cider-ns.el

f38872c

See #2314 for the rationale behind this change. Basically we plan to put several ns manipulation commands together in the same module.

arichiardi force-pushed the require-reload branch from 316da57 to 198f41f Compare July 22, 2018 20:20

arichiardi changed the title ~~Add cider-reload-ns interactive command~~ Add cider-reload-ns and cider-ns-reload-all interactive command Aug 1, 2018

arichiardi force-pushed the require-reload branch from 198f41f to fc0899f Compare August 1, 2018 16:45

bbatsov reviewed Aug 2, 2018

View reviewed changes

vspinu reviewed Aug 2, 2018

View reviewed changes

Add cider-ns-reload and cider-ns-reload-all commands

686f783

The C-c M-n l has been assigned to cider-ns-reload while C-c M-n M-l has been assigned to cider-ns-reload-all.

arichiardi force-pushed the require-reload branch from fc0899f to 686f783 Compare August 2, 2018 16:52

bbatsov merged commit 623a669 into clojure-emacs:master Aug 2, 2018

arichiardi deleted the require-reload branch August 3, 2018 17:30


		The prefix argument ARG can change the behavior of the command:

		- (4) \\[cider-reload]: prompts for a namespace name.

Add cider-reload-ns and cider-ns-reload-all interactive command #2314

Add cider-reload-ns and cider-ns-reload-all interactive command #2314

Conversation

arichiardi commented Jun 8, 2018 • edited Loading

vspinu commented Jun 8, 2018

arichiardi commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi commented Jun 9, 2018

bbatsov commented Jun 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspinu commented Jun 18, 2018

arichiardi commented Jun 18, 2018

bbatsov commented Jun 21, 2018

arichiardi commented Jul 1, 2018 • edited Loading

bbatsov commented Jul 1, 2018

arichiardi commented Jul 1, 2018 • edited Loading

arichiardi commented Jul 1, 2018

arichiardi commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi commented Jul 22, 2018

bbatsov commented Jul 23, 2018

arichiardi commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspinu commented Aug 2, 2018

bbatsov commented Aug 2, 2018

arichiardi commented Aug 2, 2018

bbatsov commented Aug 2, 2018

arichiardi commented Aug 2, 2018

arichiardi commented Jun 8, 2018 •

edited

Loading

arichiardi commented Jul 1, 2018 •

edited

Loading

arichiardi commented Jul 1, 2018 •

edited

Loading