search

trptcolin / reply

REPL-y: A fitter, happier, more productive REPL for Clojure.
Eclipse Public License 1.0
337 stars 44 forks source link

Fix exception when attaching to tools.nrepl remote nREPL #195

Closed mrichards42 closed 5 years ago

mrichards42 commented 5 years ago

If I try to connect to a remote nREPL that uses the old tools.nrepl instead of the new nrepl, I get the following exception:

$ java -jar target/reply-0.4.4-SNAPSHOT-standalone.jar --attach localhost:6000
CompilerException java.lang.ClassNotFoundException: nrepl.core, compiling:(/tmp/form-init5773107386382421609.clj:1:91)
#object[clojure.lang.Namespace 0x5dc2b378 "user"]
Error loading namespace; falling back to user
nil
user=>

This PR fixes the exception by resolving the nREPL version locally instead of in the remote repl.

bbatsov commented 5 years ago

Isn’t it easier for you to just upgrade the remote REPL?

trptcolin commented 5 years ago

Yeah... I wonder if it'd be useful to print both the local & remote versions. Seems like info about a version number mismatch could be valuable in the future if there were ever breaking changes.

bbatsov commented 5 years ago

Don't you normally care only about the version of the REPL you're connected to?

mrichards42 commented 5 years ago

It's fine by me if reply doesn't want to support connecting to a remote repl with a different nrepl version than the client, and I don't mind this PR being closed. It seemed like a small enough change to be worth thinking about, especially since the only problem is in the purely informational prelude.

I agree though that the remote repl and clojure version are the important things to print, and it's possible that future nrepl version differences might be great enough that it doesn't make sense to even try connecting if the remote repl version is different enough.

bbatsov commented 5 years ago

I think you missed my point - I was wondering if there's ever a case when we care about the version of nREPL bundled with REPLy. After you all you always have to connect to nREPL, even if you spin it from REPLy. In the end of the day the remote version is that one that counts. Your suggestion would return a version that's misleading (as you won't actually be interaction with nREPL of that version), that's why I'm not sure that's a good idea.

mrichards42 commented 5 years ago

No, I follow. I agree that the important information is the remote nREPL version, and I at this point I agree that the fix suggested in this PR is not the right approach.

It could make sense to wrap the prelude in a try-catch since it's just printing information. But since this problem is only going to affect the one-time switch from tools.nrepl to nrepl, it's not a big deal to me.