Add refresh middleware

[cichli]

This just delegates to the eval op, similar to nREPL's own load-file. Unfortunately it's tricky to call refresh directly from the middleware since it sets *e, which doesn't play well with nREPL's binding conveyance. Seeing as how load-file also sets *e and *1 I think it's acceptable for this middleware to do it too.

The msg arg to refresh-code is unused as of now, but I plan to add after/before options to make the reloaded workflow easier to integrate in CIDER - I'm just not sure how this will work client-side. I guess we can prompt for the namespace-qualified symbols of the functions to call before/after refreshing, or have options for them and encourage people to use .dir-locals.el to set them on a per-project basis.

Fixes #13.

[cichli]

Looking into the Travis failure now.

[bbatsov]

The msg arg to refresh-code is unused as of now, but I plan to add after/before options to make the reloaded workflow easier to integrate in CIDER - I'm just not sure how this will work client-side. I guess we can prompt for the namespace-qualified symbols of the functions to call before/after refreshing, or have options for them and encourage people to use .dir-locals.el to set them on a per-project basis.

Sounds legit.

[cichli]

Okay, the tests were failing because of the hardcoded 1 second timeout in session-fixture. Hence the indeterminism. Knew that would come back to bite me eventually :-). I've set it to Long/MAX_VALUE, as in nREPL's own tests.

[vspinu]

I wonder if "refresh" should simply be an op in "ns" middleware. There is also "refresh-all" along other cool stuff in tools.namespace which could be added later.

[cichli]

I wonder if "refresh" should simply be an op in "ns" middleware

There'd be no harm in that once #143 is resolved - but until then I think it's a good idea to keep the tools.namespace dependency separate (see #113).

refresh-all would definitely be a useful addition, good catch - I'll add it in a separate PR. I guess providing a prefix arg to cider-refresh could be how you invoke it?

[vspinu]

I think it's a good idea to keep the tools.namespace dependency separate (see #113).

I don't quite follow. You don't have an explicit dependency in refresh.clj. Same applies if the code is moved to ns.clj.

BTW, to me it feels that refresh/refresh-all terminology is not that intuitive. load-changed and load-all are much better IMO. So maybe have load middleware with load-changed and load-all ops? There will surely be other "loadings" in the future.

refresh-all would definitely be a useful addition, good catch - I'll add it in a separate PR. I guess providing a prefix arg to cider-refresh could be how you invoke it?

Sounds good.

Alternatively, one can already think of a specialized keymap for ns-related manipulation ( C-c C-s in clojure-emacs/cider#692 proposal) and have separate bindings for them.

[cichli]

I don't quite follow. You don't have an explicit dependency in refresh.clj. Same applies if the code is moved to ns.clj.

Yes, but the op will only work in a project with tools.namespace included - and to be excludable, it has to be behind a separate handler to the other ns ops. Thinking about it a bit more, even once #143 is resolved, this op should still be excludable separately (because we'd still have to eval refresh in the user's project, which is what causes the breakage in #113). Currently the project is structured with one handler per namespace, but maybe in this case it makes more sense to put these two handlers in the same place.

Regarding the naming: I picked the same names as tools.namespace as it's already a well-known library. But I definitely agree that to someone unaware of that, the names would be unintuitive. refresh doesn't just load changed files, it loads anything not yet loaded on the classpath. Maybe reload and load-all would be more suggestive?

Let's see what @bbatsov thinks.

[bbatsov]

Currently the project is structured with one handler per namespace, but maybe in this case it makes more sense to put these two handlers in the same place.

I'm not opposed to this. The current structure is mostly incidental.

Regarding the naming: I picked the same names as tools.namespace as it's already a well-known library. But I definitely agree that to someone unaware of that, the names would be unintuitive. refresh doesn't just load changed files, it loads anything not yet loaded on the classpath. Maybe reload and load-all would be more suggestive?

Not sure. I think refresh is pretty popular term amongst Clojurists. Btw, when I was thinking of writing this my idea was to leverage lower-level functions (not refresh directly), so I could implement a nicer UI feedback for the Emacs command. That's why right I simply use eval - as it seemed to me middleware as the one proposed right now offers little advantages over eval.

[bbatsov]

Any thoughts from anyone?

[expez]

I think both names have merit. refresh is easier for those already familiar with clojure tooling, and the reloaded workflow in particular, and load would be better for everyone else. I think this discussion is more important when exposing an op in cider itself than in this project.

As to the structuring of the handlers I feel like the nrepl stuff kind of belongs at the outer edges. Now you'd have to grep through the project to find all the exposed ops and where they are, which I think is annoying.

In trying to split refactor-nrepl into refactor-nrepl and refactor-nrepl-core I have a middleware.clj file which essentially pulls in all functionality from core and, among other things, has this snippet:

(def refactor-nrepl-ops
  {"resolve-missing" resolve-missing-reply
   "find-debug-fns" find-debug-fns-reply
   "find-symbol" find-symbol-reply
   "artifact-list" artifact-list-reply
   "artifact-versions" artifact-versions-reply
   "hotload-dependency" hotload-dependency-reply
   "clean-ns" clean-ns-reply
   "find-unbound" find-unbound-reply})

(defn wrap-refactor
  [handler]
  (fn [{:keys [op] :as msg}]
    ((get refactor-nrepl-ops op handler) msg)))

(set-descriptor!
 #'wrap-refactor
 {:handles (zipmap (keys refactor-nrepl-ops)
                   (repeat {:doc "See the refactor-nrepl readme"}))})

From this one file you can start jumping around to look at whatever op is of interest. I also found the descriptors to be a pain in the butt, essentially duplicating the readme. I can't imagine anyone actually consuming those descriptors over consulting the readme or the code itself.

I just thought I'd throw this out there. The good news is we can change this around as much as we'd like because it doesn't affect the public API.

[bbatsov]

@cichli Ping :-)

What I'm mostly interested right now is whether you'd like to implement a more low-level version of the this that'd support:

• checking which namespaces would get reloaded
• returning a list of reloaded namespaces we can display in some nicer way than just a minibuffer message (eventually)

[cichli]

@bbatsov Sorry, I've not forgotten about this :-). The first part is easy, but do we want our reloading to have the same features as tools.namespace.repl/refresh?

i.e.

• restore any aliases/refers in *ns* if reloading fails
• allow for disabling of unloading/reloading on individual namespaces

Both are doable but may involve using a couple of private functions in tools.namespace.repl.

[bbatsov]

That'd be nice to have, although not a hard requirement.

Both are doable but may involve using a couple of private functions in tools.namespace.repl.

Guess I can live with this. :-)

[cichli]

Okay, I've pushed a new stab at this that uses tools.namespace directly. It works well from my manual testing, but there are a few outstanding issues:

• Bad error conveyance - currently the response only includes the namespace that caused the error to be thrown. For now, I think the best thing to do is require cider.nrepl.middleware.stacktrace and use analyze-causes to include the full stacktrace data in the error response, and have the handler on the client side know to expect this. Not ideal, but right now the stacktrace middleware just checks *e when using the stacktrace op, which we obviously don't want to set.
• Not really sure how to handle error recovery in the same way as tools.namespace in the face of multiple nREPL sessions. Should we just enumerate the set of distinct values of *ns* for all sessions and do the recovery for each? Note that clojure.tools.nrepl.middleware.session/sessions is private.
• Running track/reload as part of the test suite can cause subsequent tests to break in weird ways, unless dirs is restricted to e.g. test/resources, but even this still seems to break cloverage entirely. I'll see if this can be fixed in cloverage but for now we should skip testing the refresh op itself.

[cichli]

returning a list of reloaded namespaces we can display in some nicer way than just a minibuffer message (eventually)

Regarding this bit, we could either have a list of namespaces or a tree of directory->namespace. Out of date directories/namespaces would be highlighted in the view of this tree, and the user could mark individual directories or namespaces for reloading/not reloading (the current implementation respects the use of clojure.tools.namespace.repl/disable-unload! and clojure.tools.namespace.repl/disable-reload!).

Thoughts?

[bbatsov]

Bad error conveyance - currently the response only includes the namespace that caused the error to be thrown. For now, I think the best thing to do is require cider.nrepl.middleware.stacktrace and use analyze-causes to include the full stacktrace data in the error response, and have the handler on the client side know to expect this. Not ideal, but right now the stacktrace middleware just checks *e when using the stacktrace op, which we obviously don't want to set.

Sounds good.

Not really sure how to handle error recovery in the same way as tools.namespace in the face of multiple nREPL sessions. Should we just enumerate the set of distinct values of ns for all sessions and do the recovery for each? Note that clojure.tools.nrepl.middleware.session/sessions is private.

What are the alternatives?

Running track/reload as part of the test suite can cause subsequent tests to break in weird ways, unless dirs is restricted to e.g. test/resources, but even this still seems to break cloverage entirely. I'll see if this can be fixed in cloverage but for now we should skip testing the refresh op itself.

Fine by me.

Regarding this bit, we could either have a list of namespaces or a tree of directory->namespace. Out of date directories/namespaces would be highlighted in the view of this tree, and the user could mark individual directories or namespaces for reloading/not reloading (the current implementation respects the use of clojure.tools.namespace.repl/disable-unload! and clojure.tools.namespace.repl/disable-reload!).

Thoughts?

Sounds amazing! Much better than the UI I had in mind. :-)

[bbatsov]

👍