Support clj-reload workflow #850

filipesilva · 2024-02-23T17:59:38Z

Before submitting a PR make sure the following things have been done:

The commits are consistent with our contribution guidelines
You've added tests to cover your change(s)
All tests are passing
You've updated the README
Middleware documentation is up to date
- Please check out and modify the cider.nrepl ns which has all middleware documentation.
- Run lein docs afterwards, and commit the results.

Note: If you're just starting out to hack on cider-nrepl you might find
nREPL's documentation and the
"Design" section of the README extremely useful.*

Thanks!

vemv

Looking good, thanks!

Some suggestions

src/cider/nrepl.clj

src/cider/nrepl/middleware/reload.clj

vemv · 2024-02-23T18:08:16Z

src/cider/nrepl/middleware/reload.clj

+          #(try
+             (let [reload (user-reload)]
+               (when-not reload
+                 (throw (ex-info "clj-reload.core/reload not found" {})))


Looks like we don't need to be this stringent, following tonsky/clj-reload@0a082e9

Oh that's right! We can use the project one if available, and fall back to the bundled one if not.

vemv · 2024-02-23T18:08:59Z

src/cider/nrepl/middleware/reload.clj

+(defn handle-reload [handler msg]
+  (case (:op msg)
+    "reload" (refresh-reply msg)
+    "reload-all" (refresh-reply (assoc msg :all true))


Is there not a clear op?

Hm no, I can't see it in https://github.com/tonsky/clj-reload/blob/main/src/clj_reload/core.clj

src/cider/nrepl/middleware/reload.clj

vemv · 2024-02-23T18:12:25Z

src/cider/nrepl/middleware/reload.clj

+               (respond msg {:status :ok}))
+             (catch Throwable error
+               (respond msg {:status :error
+                             :error  (stacktrace.analyzer/analyze error print-fn)})


Suggested change

:error (stacktrace.analyzer/analyze error print-fn)})

;; TODO assoc :file, :line info if available

:error (stacktrace.analyzer/analyze error print-fn)})

I'd like this to eventually return file/line info if we can easily grab it from the analyzer. This way cider.el could automatically open the culprit file.

(Feel free to give it a shot, but simply adding the TODO is perfectly fine)

Added the TODO for now, but may circle back later if I can manage it.

src/cider/nrepl.clj

src/cider/nrepl/middleware/reload.clj

filipesilva · 2024-02-23T20:29:49Z

@vemv running PARSER_TARGET=parser-next make test fails with:

Running tests in #{"/Users/filipesilva/work/cider-nrepl/test/spec" "/Users/filipesilva/work/cider-nrepl/test/clj" "/Users/filipesilva/work/cider-nrepl/test/common" "/Users/filipesilva/work/cider-nrepl/test/cljs"}
Execution error (FileNotFoundException) at cider.nrepl.middleware.reload-test/eval21172$loading (reload_test.clj:1).
Could not locate clj_reload/core__init.class, clj_reload/core.clj or clj_reload/core.cljc on classpath. Please check that namespaces with dashes use underscores in the Clojure file name.

But PARSER_TARGET=parser-next make fast-test does not fail. Do you know why?

vemv · 2024-02-23T20:37:31Z

Thanks for applying the feedback!

I think it's a known issue with mranderson - see:

cider-nrepl/test/clj/cider/nrepl/middleware/refresh_test.clj

Lines 131 to 136 in 3824d72

    
           ;; Disabling the next test. 
        
           ;; Unclear how to get the "real" clojure.tools.namespace.repl in 
        
           ;; this test when this project also localizes via mranderson. 
        
           #_(testing "honors set-refresh-dirs" 
        
               (c.t.n.r/set-refresh-dirs "foo" "bar") 
        
               (is (= ["foo" "bar"] (#'r/user-refresh-dirs)))))

You could perform the require dynamically, only running the deftest if (try (require 'clj-reload.core) true (catch Exception _ false)) ;; only run the deftest if inlining hasn't been performed

filipesilva · 2024-02-23T21:51:25Z

@vemv all the tests in cider.nrepl.middleware.reload-test depend on the init, so disabling it means it wouldn't run at all in make test... WDYT of using init from the cider.nrepl.middleware.reload ns like I have in this last commit?

filipesilva · 2024-02-23T22:36:37Z

That last commit seems to have sorted the cli-reload errors. But now a see two errors that I really don't understand....

FAIL in (instrument-function-call-test) (instrument_test.clj:109)
expected: #{[(System/currentTimeMillis) [3 1 1]] [(Thread/sleep 1000) [3 2]] [(- (bp (System/currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) [3 3]] [(System/currentTimeMillis) [3 3 1]] [(def test-fn (fn* ([] (bp (let* [start-time (bp (System/currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (Thread/sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (System/currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) {:coor [3]} (let [start-time (System/currentTimeMillis)] (Thread/sleep 1000) (- (System/currentTimeMillis) start-time)))))) []] [start-time [3 3 2]] [(let* [start-time (bp (System/currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (Thread/sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (System/currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) [3]]}
  actual: #{[(- (bp (. System currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) [3 3]] [(let* [start-time (bp (. System currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (. Thread sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (. System currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) [3]] [(. System currentTimeMillis) [3 1 1]] [(. Thread sleep 1000) [3 2]] [start-time [3 3 2]] [(. System currentTimeMillis) [3 3 1]] [(def test-fn (fn* ([] (bp (let* [start-time (bp (. System currentTimeMillis) {:coor [3 1 1]} (System/currentTimeMillis))] (bp (. Thread sleep 1000) {:coor [3 2]} (Thread/sleep 1000)) (bp (- (bp (. System currentTimeMillis) {:coor [3 3 1]} (System/currentTimeMillis)) (bp start-time {:coor [3 3 2]} start-time)) {:coor [3 3]} (- (System/currentTimeMillis) start-time))) {:coor [3]} (let [start-time (System/currentTimeMillis)] (Thread/sleep 1000) (- (System/currentTimeMillis) start-time)))))) []]}

FAIL in (inspect-var-integration-test) (inspect_test.clj:264)
rendering a var
expected: (= var-result (value (session/message {:op "eval", :inspect "true", :code "#'*assert*"})))
  actual: (not (= ("Class" ": " (:value "clojure.lang.Var" 0) (:newline) "Value: " (:value "true" 1) (:newline) (:newline) "--- Meta Information:" (:newline) "  " (:value ":ns" 2) " = " (:value "clojure.core" 3) (:newline) "  " (:value ":name" 4) " = " (:value "*assert*" 5) (:newline) (:newline) "--- Datafy:" (:newline) "  " "0" ". " (:value "true" 6) (:newline)) ("Class" ": " (:value "clojure.lang.Var" 0) (:newline) "Value: " (:value "true" 1) (:newline) (:newline) "--- Meta Information:" (:newline) "  " (:value ":added" 2) " = " (:value "\"1.0\"" 3) (:newline) "  " (:value ":ns" 4) " = " (:value "clojure.core" 5) (:newline) "  " (:value ":name" 6) " = " (:value "*assert*" 7) (:newline) "  " (:value ":doc" 8) " = " (:value "\"When set to logical false, 'assert' will omit assertion checks in\\n  compiled code. Defaults to true.\"" 9) (:newline) (:newline) "--- Datafy:" (:newline) "  " "0" ". " (:value "true" 10) (:newline))))

I didn't touch or I think interact with these ns's in any way, do you have any idea of what's happening?

vemv · 2024-02-25T11:02:14Z

Yes, feel free to disregard those - we can fix them before we cut a stable release.

Is there anything pending from your side?

filipesilva · 2024-02-25T17:58:45Z

Should I do any changes to the README, as the PR template mentions?

I'd like to test locally a bit to make sure everything is fine, but hitting a failure... when I run

# Install the project in your local ~/.m2 directory, using mranderson (recommended)
# The JVM flag is a temporary workaround.
export LEIN_JVM_OPTS="-Dmranderson.internal.no-parallelism=true"
PROJECT_VERSION=0.45.0-reload make install

it fails with this message:

...
prefixing #{"difflib"} in target/class-deps.jar with cidernrepl0450reload
deleting directories with class files in target/srcdeps...
   difflib  deleted
unzipping repackaged class-deps.jar into target/srcdeps
touch .inline-deps
rm -f .no-mranderson
touch .no-pedantic
lein with-profile -user,-dev,+1.11,+plugin.mranderson/config install
Error: Could not find or load main class clojure.main
Caused by: java.lang.ClassNotFoundException: clojure.main
Compilation failed: Subprocess failed (exit code: 1)
Error encountered performing task 'install' with profile(s): 'base,system,provided,1.11,config'
Compilation failed: Subprocess failed (exit code: 1)
make: *** [install] Error 1

Am I doing something wrong? If there's a better way to test locally let me know.

vemv · 2024-02-25T19:01:05Z

That looks like an occurrence of #848 - please try with a slightly older Lein?

filipesilva · 2024-02-25T20:22:18Z

Yeap that worked!

... but I'm at a bit of a loss of how to test it now. I mean CIDER won't have this operation, right? What should I do to test it in CIDER?

filipesilva · 2024-02-25T20:26:55Z

Well if there's nothing more that I can test at this point, I don't think there's anything pending on my side. Just pushed the README update.

vemv · 2024-02-25T20:32:51Z

Happy that you got it!

After a successful make install, you could fork cider https://docs.cider.mx/cider/contributing/hacking.html so that the new ops are used insted of the tools.namespace ones.

There should be a few defcustoms e.g. cider-refresh-op, cider-refresh-all-op.

Should be a small change overall.

It also should be good timing to discuss upstream whether a clear function op would make sense for clj-reload. Else we'd have to void that op somehow, to avoid surprises.

filipesilva · 2024-03-02T10:30:15Z

What's the difference between the clear op and the cider-refresh-all-op?

I tried to setup CIDER locally but get a native compile error while doing eldev build :autoloads. I tried something different though: I went into nrepl.clj, changed (def-wrapper wrap-refresh cider.nrepl.middleware.refresh/handle-refresh to use cider.nrepl.middleware.reload/handle-reload instead, update handle-reload to handle the refresh op, make install, then load it in a project manually instead of via jack-in.

I could see cider-ns-refresh working and logging messages Reloading successful, but the current refresh functionality logs this message anyway. There's a cider-ns-reload-all but that doesn't actually seem related to the cider-ns-refresh, at least from the doc. I don't think refresh-all or refresh-clear are currently exposed as CIDER commands.

This is the test I used:

(def foo 1)

(defonce bar 5)

(comment
  foo
  bar)

I saved, reloaded, and got eval'ed foo and bar in the comment block. Then altered their values, reloaded again, and saw foo update the value but bar remained. This is a different in cli-reload than in clojure.tools.namespace.repl: the former keeps defonce, but the latter does not. So I think this updated cider-ns-refresh is working with cli-reload. I double checked this test and with release version of CIDER the refresh does indeed update the value of the defonce var.

filipesilva · 2024-03-02T10:37:47Z

What are you thoughts on using the new operation from within CIDER by the way? I guessed that a new command would be added, cider-ns-reload would be added, but there's already a cider-ns-reload. Maybe cider-ns-clj-reload?

vemv · 2024-03-02T10:59:06Z

Hi Filipe!

What's the difference between the clear op and the cider-refresh-all-op?

In general, clear unloads all code, without trying to load it again. This is useful in case something temporarily broke - then one can clear, fix the code, load it by hand, ensure it works, then refresh to load the rest of the proect.

I don't think refresh-all or refresh-clear are currently exposed as CIDER commands.

Looks like they are: https://github.com/clojure-emacs/cider/blob/dc58ed1a411e6fe6f0350435aa4c56ec6edf59bf/cider-ns.el#L257-L261

What are you thoughts on using the new operation from within CIDER by the way? I guessed that a new command would be added

I'd prefer if the defuns/commands remained the same. Instead, the ops would be customizable via defcustoms.

Cheers - V

filipesilva · 2024-03-02T11:13:46Z

Ok so what's next? I don't think there's anything left for me to do in this PR, or is there?

vemv · 2024-03-02T11:40:15Z

I'd like to know if there's an intention from clj-reload's side to offer clear functionality.

In the meantime, WIP PR welcome for https://github.com/clojure-emacs/cider - let us know if it's working as intended, using the suggested defcustom design.

filipesilva · 2024-03-02T11:42:31Z

I see now all the refresh nrepl operations are within the same cider-ns-refresh, that's what I missed.

It also makes a lot more sense why you'd like
clear to also exist, that way the semantics of the operation are maintained.

But I don't see an exposed unload operation in clj-reload, it's not part of the API I think.

Maybe we could leave that one as not yet implemented and the interface would still be the same?

filipesilva · 2024-03-02T11:45:15Z

@tonsky what are your thoughts about a clear/unload operation?

tonsky · 2024-03-02T17:06:38Z

In general, clear unloads all code, without trying to load it again. This is useful in case something temporarily broke - then one can clear, fix the code, load it by hand, ensure it works, then refresh to load the rest of the proect.

This? We can do this, sure. Send a PR or I’ll do it myself next week

See clojure-emacs/cider-nrepl#850 for more.

filipesilva · 2024-03-02T22:56:18Z

@vemv early draft of the cider changes in clojure-emacs/cider#3624, PTAL if it matches what you had in mind.

See clojure-emacs/cider-nrepl#850 for more.

filipesilva · 2024-03-03T15:16:38Z

src/cider/nrepl/middleware/reload.clj

+  (case (:op msg)
+    "cider.clj-reload/reload" (refresh-reply msg)
+    "cider.clj-reload/reload-all" (refresh-reply (assoc msg :all true))
+    "cider.clj-reload/reload-clear" (refresh-reply (assoc msg :clear true))


@vemv left support for the clear op here so it exists when called from CIDER, but it's still not functional as per the TODO in this file.

filipesilva · 2024-03-03T15:19:06Z

src/cider/nrepl/middleware/reload.clj

+    (exec id
+          (fn []
+            (try
+              ;; TODO: clear op


@tonsky I think I'll wait until you implement the clear op, because I felt my init implementation was a rather far from what you ended up merging and unload seems to be more involved than init.

I think everything else is done though. Today I tested the e2e of the feature in my emacs and it was working as expected. All users need to do is add (setq cider-ns-refresh-tool 'clj-reload).

tonsky · 2024-03-04T18:40:04Z

Here you go https://github.com/tonsky/clj-reload/releases/tag/0.4.0

vemv · 2024-03-04T18:43:42Z

Thanks much, @tonsky 🎉!

@filipesilva , if you merge master in the build will become green.

Be sure to add clear, test it out locally, refresh docs and we're good to go.

src/cider/nrepl/middleware/reload.clj

bbatsov · 2024-03-04T18:59:03Z

src/cider/nrepl/middleware/reload.clj

+  [{:keys [transport] :as msg} response]
+  (transport/send transport (response-for msg response)))
+
+(defn- refresh-reply


I guess you forgot to rename this.

Yeap, initial copy paste then didn't look at it again :(

bbatsov · 2024-03-04T19:04:39Z

Yeah, it'd be nice to explain both here and in CIDER's docs (later) why are there 2 instances of relatively similar functionality. I'm guessing if clj-reload works well down the road we can remove c.t.n. as I don't think we need both of them in the long run and I like that clj-reload is smaller in terms of scope.

vemv · 2024-03-04T19:11:17Z

tbh that would be one big breaking change :) it's easy to imagine a variety of commercial projects that have set up the t.n boilerplate, so using t.n would be a natural fit.

Ultimately, let's see how it all plays out over the years - I reckon that only one of both projects will remain actively maintained / used. It's pretty early to bet on either.

bbatsov · 2024-03-04T19:17:52Z

tbh that would be one big breaking change :) it's easy to imagine a variety of commercial projects that have set up the t.n boilerplate, so using t.n would be a natural fit.

Yeah, I'm well aware of this. I don't see this happening soon, but I also dislike functionality duplication. (it tends to be confusing for the end users and it's more maintenance work for us) Now CIDER will have 3 ways to do code reloading, so can understand my desire for this not to be a permanent situation. Anyways, that's not particularly important for this PR.

Fix clojure-emacs#849

filipesilva · 2024-03-04T22:07:02Z

Added clear (unload in clj-reload) support, rebase, and incorporated @bbatsov's feedback.

Edit: also added a bit more about why you might want cld-reload to the cider docs in the other PR.

See clojure-emacs/cider-nrepl#850 for more.

vemv · 2024-03-05T03:55:38Z

Great work, thanks!

FYI a changelog entry was missing - no biggie, adding it now

See clojure-emacs/cider-nrepl#850 for more.

cichli · 2024-05-10T23:44:54Z

In general, clear unloads all code, without trying to load it again. This is useful in case something temporarily broke - then one can clear, fix the code, load it by hand, ensure it works, then refresh to load the rest of the proect.

clear just clears the state of the refresh tracker – see the doc for clojure.tools.namespace.repl/clear – it forces the next refresh to load everything, but doesn't unload anything itself. It's for recovering from corrupted states (normally caused by deleted files or circular dependencies). I think unloading is generally more useful and easier to understand, but we could maybe consider aliasing it to unload in the new middleware (sorry this reply is too late to suggest renaming it).

bbatsov · 2024-05-11T05:36:00Z

@cichli Might be good to file this as a ticket, so the suggestion to add an alias won't be forgotten.

filipesilva force-pushed the reload branch from a86f61a to 48f8165 Compare February 23, 2024 18:00

filipesilva mentioned this pull request Feb 23, 2024

Support for custom cider-ns-refresh-fn #849

Closed

vemv reviewed Feb 23, 2024

View reviewed changes

filipesilva force-pushed the reload branch 6 times, most recently from a42b6dd to 37a8dbf Compare February 23, 2024 20:23

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 2, 2024

Support cider.clj-reload/reload ops

2b67373

See clojure-emacs/cider-nrepl#850 for more.

filipesilva mentioned this pull request Mar 2, 2024

Support cider.clj-reload/reload ops clojure-emacs/cider#3624

Closed

6 tasks

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 3, 2024

Support cider.clj-reload/reload ops

afb4297

See clojure-emacs/cider-nrepl#850 for more.

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 3, 2024

Support cider.clj-reload/reload ops

49e91b5

See clojure-emacs/cider-nrepl#850 for more.

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 3, 2024

Support cider.clj-reload/reload ops

194877f

See clojure-emacs/cider-nrepl#850 for more.

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 3, 2024

Support cider.clj-reload/reload ops

21e62e0

See clojure-emacs/cider-nrepl#850 for more.

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 3, 2024

Support cider.clj-reload/reload ops

e707d10

See clojure-emacs/cider-nrepl#850 for more.

filipesilva force-pushed the reload branch from a54f25d to 958dfca Compare March 3, 2024 15:15

filipesilva commented Mar 3, 2024

View reviewed changes

bbatsov reviewed Mar 4, 2024

View reviewed changes

src/cider/nrepl/middleware/reload.clj Show resolved Hide resolved

bbatsov reviewed Mar 4, 2024

View reviewed changes

Support clj-reload workflow

78ac4e4

Fix clojure-emacs#849

filipesilva force-pushed the reload branch from 958dfca to 78ac4e4 Compare March 4, 2024 21:51

filipesilva marked this pull request as ready for review March 4, 2024 22:06

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 4, 2024

Support cider.clj-reload/reload ops

25148d8

See clojure-emacs/cider-nrepl#850 for more.

vemv merged commit d8fbd01 into clojure-emacs:master Mar 5, 2024
63 checks passed

filipesilva added a commit to filipesilva/cider that referenced this pull request Mar 9, 2024

Support cider.clj-reload/reload ops

23b1445

See clojure-emacs/cider-nrepl#850 for more.

	:error (stacktrace.analyzer/analyze error print-fn)})
	;; TODO assoc :file, :line info if available
	:error (stacktrace.analyzer/analyze error print-fn)})

Support clj-reload workflow #850

Support clj-reload workflow #850

Conversation

filipesilva commented Feb 23, 2024 • edited Loading

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv Feb 23, 2024 • edited Loading

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

filipesilva commented Feb 23, 2024

vemv commented Feb 23, 2024

filipesilva commented Feb 23, 2024

filipesilva commented Feb 23, 2024

vemv commented Feb 25, 2024

filipesilva commented Feb 25, 2024

vemv commented Feb 25, 2024

filipesilva commented Feb 25, 2024

filipesilva commented Feb 25, 2024

vemv commented Feb 25, 2024 • edited Loading

filipesilva commented Mar 2, 2024

filipesilva commented Mar 2, 2024

vemv commented Mar 2, 2024

filipesilva commented Mar 2, 2024

vemv commented Mar 2, 2024

filipesilva commented Mar 2, 2024

filipesilva commented Mar 2, 2024

tonsky commented Mar 2, 2024

filipesilva commented Mar 2, 2024

Choose a reason for hiding this comment

filipesilva Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

tonsky commented Mar 4, 2024

vemv commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Mar 4, 2024

vemv commented Mar 4, 2024

bbatsov commented Mar 4, 2024 • edited Loading

filipesilva commented Mar 4, 2024 • edited Loading

vemv commented Mar 5, 2024

cichli commented May 10, 2024

bbatsov commented May 11, 2024

filipesilva commented Feb 23, 2024 •

edited

Loading

vemv Feb 23, 2024 •

edited

Loading

vemv commented Feb 25, 2024 •

edited

Loading

filipesilva Mar 3, 2024 •

edited

Loading

bbatsov commented Mar 4, 2024 •

edited

Loading

filipesilva commented Mar 4, 2024 •

edited

Loading