search

sanel / monroe

Clojure nREPL client for Emacs
161 stars 21 forks source link

Add M-. and M-, for jumping to and from var definitions. #15

Closed technomancy closed 7 years ago

technomancy commented 7 years ago

Sorry; I noticed a bug in how it handles files inside jars. I will push a fix and let you know.

sanel commented 7 years ago

Great! Was planning to review the patch, but you were faster. I'll wait the fix.

technomancy commented 7 years ago

This should fix the issues I found.

sanel commented 7 years ago

Sorry for delay :)

I've noticed you are using clojure.java.io; how this will affect clojurescript?

technomancy commented 7 years ago

Oh, I didn't realize monroe worked with clojurescript. I've never used cljs before; does it have some equivalent of clojure.java.io/resource, or does it need some different logic to find the definition of a var?

sanel commented 7 years ago

AFAIK there is no equivalent and I believe CIDER is using middleware for this, but I might be wrong.

technomancy commented 7 years ago

I see. Would you rather have it no-op out under cljs for now, or do you have something better in mind?

Unrelatedly, I found a bug in the M-, implementation, so I need to fix that before this gets merged.

technomancy commented 7 years ago

OK, this fixes the M-, bug and also introduces monroe-translate-path-function which allows you to apply path transformations in case the paths in your nrepl server are remote or containerized.

technomancy commented 7 years ago

OK, this is now a no-op if clojure.java.io/resource does not resolve, for platforms like clojurescript.

bbatsov commented 7 years ago

AFAIK there is no equivalent and I believe CIDER is using middleware for this, but I might be wrong.

Yeah, it does.

bbatsov commented 7 years ago

CIDER had pretty much the same code before the middleware era. :-) Your discussion about the cljs support reminded me of the reasons that lead to dropping this model (with the inlined code). It's simple to just eval code, but it's also very limiting...

sanel commented 7 years ago

Thanks guys for the effort and @bbatsov nice to have you here :) I'll review it soon.

sanel commented 7 years ago

@technomancy resolve doesn't work on clojurescript. AFAIK, solution could be to use reader conditionals, but that will break on clojure older than 1.7.

It's simple to just eval code, but it's also very limiting...

@bbatsov not perfect, true. IMO this should not be much of the code except to figure out presence of particular environment. However, if it starts to drag a bit of code, I'd go with separate plugin or addon...

bbatsov commented 7 years ago

AFAIK, solution could be to use reader conditionals, but that will break on clojure older than 1.7.

Btw, ClojureScript is 1.7+ only, so if you want to support it properly it makes sense to target 1.7+. That's why CIDER doesn't support older Clojure releases anymore.

I'm guessing you can also have some conditional logic about the code you're trying to evaluate based on the version of the Clojure runtime.

technomancy commented 7 years ago

TBH I don't think it's worth expending any effort on detecting whether we're in an environment that supports finding the location of vars or not. We already set up an event handler that discards error output. If we send a "tell me the location where this var is defined" to a server that doesn't support it, and we get an error back, we do nothing, which is exactly the same as what we would do if we could somehow construct a form that would be portable across all known Clojurelike nrepl servers, except the latter approach would still fail on weird nrepl servers like Jeejah.

Though I did notice the jump handler was missing a clause to handle status=done messages (I assumed that was handled upstream) so now it will clean up stale handler functions correctly.

sanel commented 7 years ago

Thanks for the patch @technomancy and your inputs @bbatsov.