[#1032] Combine jump-to-var and jump-to-resource into one function. #1036

EricGebhart · 2015-03-20T16:44:01Z

This adds an entirely new function jump-to-resource-or-var.
A prompt is given with thing at point as the default. Thing-at-point uses 'filename as
that is more leniant than 'symbol.
First an attempt is made to find the resource. Upon failure jump to var is attempted.
C-u prefix will cause the result to go to other-window.

bbatsov · 2015-03-22T15:26:40Z

cider-interaction.el

@@ -762,6 +762,32 @@ window."
      (when pos
        (goto-char pos)))))

+(defun cider-jump-to-resource-or-var (symbol-file &optional other-window)


Seems like a bad idea to allow an optional argument in this interactive command, given that this is actually used only by another interactive command.

bbatsov · 2015-03-22T15:28:48Z

This adds an entirely new function jump-to-resource-or-var.

Without changing the existing bindings this command will unlikely be used by someone. Maybe the existing commands should be make ordinary functions.

bbatsov · 2015-03-22T15:31:56Z

A further optimization might be to check if the point is within a string - then it would make sense to use jump to resource, otherwise we would do var jump.

EricGebhart · 2015-03-23T16:05:52Z

The optional parameter is a leftover from trying to continue using cider-read-symbol-name.
I'll remove it and add your other suggestions.

EricGebhart · 2015-03-23T17:41:06Z

ok. jump-to-var and jump-to-resource are regular functions. I changed the key bindings to point at jump-to-resource-or-var. Which means there are duplicate key bindings for each.

I didn't want to break anything. Your call.

bbatsov · 2015-03-28T06:41:47Z

CHANGELOG.md

@@ -30,6 +30,8 @@
 * [#958](https://github.com/clojure-emacs/cider/pull/958) Reuse existing repl
  buffers with dead processes. Users are now informed about existing zombie repl
  buffers and are offered the choice to reuse those for new connections.
+* [#1032](https://github.com/clojure-emacs/cider/issues/1032) New function jump-to-resource-or-var 


Use full function names here and surround them with backticks.

bbatsov · 2015-03-28T06:49:24Z

I'm also starting to think that the "jump" terminology was a bad idea. Right now I prefer "lookup".

@cichli @vspinu Any feedback?

expez · 2015-03-28T08:09:09Z

How about find- now that these commands are more like find-variable, find-function etc in emacs? I think lookup is better suited for actual lookups, like the grimoire command

bbatsov · 2015-03-28T08:10:58Z

How about find- now that these commands are more like find-variable, find-function etc in emacs?

Good point! Let's use find-var, find-resource and find-dwim.

bbatsov · 2015-03-28T09:11:53Z

Btw, just realized the downside of having a combined command - what are we going to use as the completion candidates when not operating at the point? find-var will obviously show known vars, find-resource will probably show nothing. For a mix of them - it's hard to determine this.

expez · 2015-03-28T09:42:41Z

Can't we just merge all known vars and all resources on classpath?

vspinu · 2015-03-28T10:02:06Z

Can't we just merge all known vars and all resources on classpath?

Yes. I think that's pretty much the only way to go. It's a bit tricky with non-ido completion. Should the resources be full file names or base names only, or maybe both.

expez · 2015-03-28T10:25:24Z

Should the resources be full file names or base names only, or maybe both.

Anything above the project root is just noise, so I think we should display it with a path relative to the root. This lets you disambiguate between say resources/ and test/resources/ if you should have similarly named files.

EricGebhart · 2015-03-29T20:54:40Z

This version is completely different from the last commit. The new cider-prompt-for-symbol functionality changed things around quite a bit.

@bbatsov I probably misunderstood your earlier suggestion about the '.' Checking for a '.' in the name would otherwise be something I would not have done. It seems a bit risky to me.

Currently the new functions are find-dwim and find-dwim-other-window. The former jump-to-var and jump-to-resource are now non-interactive functions find-var and find-resource. The string match on '.' is still there.

bbatsov · 2015-03-30T05:47:06Z

The completion problem remains - cider-find-var used to have a smart completion and the new command has zilch. Let's do this in two steps. Rework the current PR into something that simply renames the existing commands to cider-find-var and cider-find-resource.

Afterwards we'll figure out how to auto-complete resources and how to combine the completion for resources with the completion for vars. As it stands I don't like the proposed command and won't accept it in its current state.

EricGebhart · 2015-03-30T20:55:47Z

I didn't think this iteration would go, but it seemed silly to continue having discussions about code that had not been rebased or integrated.

The completion and history for vars is easy, resources never had completion or history. I already had that working by the time I read this, so I'll push that and see what you say. I haven't had much time to look at this today.

EricGebhart · 2015-03-30T21:22:08Z

This does of course still leave adding completions for resources to be done. But it does provide the same functionality as before with the added benefit of history for resources albeit mixed with history for vars.

bbatsov · 2015-03-31T06:02:41Z

I didn't think this iteration would go, but it seemed silly to continue having discussions about code that had not been rebased or integrated.

Therefore my suggestion to do several small changes instead of focusing on merging find-var and find-resource directly.

The completion and history for vars is easy, resources never had completion or history.

Is it really easy? I've changed it several times over the years and while the current version is pretty nice (it simply reuses the REPL completion), it's not exactly ido friendly (and probably never will be). We cannot just dump all the candidates, as we can do for Emacs Lisp. But we can do this for the resources...

EricGebhart · 2015-03-31T07:00:54Z

Well, it is easy to reuse what you've already been using. Adding completion for resources will take more.

EricGebhart · 2015-03-31T18:12:58Z

What is missing in the current functionality of find-dwim verses find-var and find-resource? I don't see anything lacking. There is completion, history, the new prompting behavior with prefix and the addition of the other-window version of the function. It seems to me that anything beyond that belongs in another PR.

bbatsov · 2015-03-31T20:08:53Z

What is missing in the current functionality of find-dwim verses find-var and find-resource? I don't see anything lacking. There is completion, history, the new prompting behavior with prefix and the addition of the other-window version of the function. It seems to me that anything beyond that belongs in another PR.

Right now you'll get var completion in the new command, which is totally unacceptable to me. The new command makes a lot of sense when operating at the point, but its usability is pretty questionable otherwise as mixing two completion sources together is generally a bad idea (not to mention one is not implemented yet). @vspinu guess you know what I mean. :-)

I understand your desire to push forward with this, but until there's a foolproof solution for the problems that bother me I'd rather have this on hold. @expez You suggested this initially, but I hope you agree with my concerns. I'm starting to think this was a bad idea and there's no sensible way to implement it. Dealing with all the added complexity just to free one keybinding is not worth it.

EricGebhart · 2015-03-31T20:29:04Z

Ok. That makes sense to me. @expez, the function will be there, so you can bind it how you want. I guess we'll see what we can do with it from here. I can probably make an other window version of find-var and find-resource with that other PR. But is that something you still want if you have this one?

expez · 2015-04-01T07:35:25Z

I agree that the dwim stuff only makes sense if we're acting on point and not prompting. I also get that mixing completion sources can be undesirable. I'm thinking we should just drop this dwim stuff now.

I really want find-var and find-resources in other window, though.

bbatsov · 2015-04-02T06:19:41Z

doc/cider-refcard.tex

@@ -51,8 +51,8 @@
 \subgroup{Navigation}

 \key{M-,}{cider-jump-back}
-\key{M-.}{cider-jump-to-var}
-\key{C-c M-.}{cider-jump-to-resource}
+\key{M-.}{cider-jump-to-resource-or-var}


You forgot to update this.

bbatsov · 2015-04-02T06:46:47Z

Overall things look good, apart from my small remarks. As discussed we're now not going to change the default keybindings, so the new commands should have keybindings of their own (or none).

…e function

EricGebhart · 2015-04-02T16:49:51Z

I must not have double checked... :-/

[#1032] Combine jump-to-var and jump-to-resource into one function.

bbatsov · 2015-04-02T16:58:41Z

👍

EricGebhart force-pushed the jump-to branch 5 times, most recently from 5fc2847 to 55b7ead Compare March 21, 2015 06:17

bbatsov reviewed Mar 22, 2015
View reviewed changes

EricGebhart force-pushed the jump-to branch from 55b7ead to af47844 Compare March 23, 2015 17:38

EricGebhart mentioned this pull request Mar 23, 2015

cider-jump with prefix to open buffer in other window? #1014

Closed

bbatsov reviewed Mar 28, 2015
View reviewed changes

EricGebhart force-pushed the jump-to branch from af47844 to b0cbe52 Compare March 29, 2015 20:50

EricGebhart force-pushed the jump-to branch from b0cbe52 to 1d7d968 Compare March 30, 2015 20:58

EricGebhart force-pushed the jump-to branch from 1d7d968 to 8a28561 Compare March 30, 2015 21:23

EricGebhart force-pushed the jump-to branch from 8a28561 to 34ef2f7 Compare March 31, 2015 21:55

bbatsov reviewed Apr 2, 2015
View reviewed changes

[clojure-emacs#1032] Combine jump-to-var and jump-to-resource into on…

5528f1e

…e function

EricGebhart force-pushed the jump-to branch from 34ef2f7 to 5528f1e Compare April 2, 2015 16:49

bbatsov added a commit that referenced this pull request Apr 2, 2015

Merge pull request #1036 from EricGebhart/jump-to

ceb2d7d

[#1032] Combine jump-to-var and jump-to-resource into one function.

bbatsov merged commit ceb2d7d into clojure-emacs:master Apr 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1032] Combine jump-to-var and jump-to-resource into one function. #1036

[#1032] Combine jump-to-var and jump-to-resource into one function. #1036

EricGebhart commented Mar 20, 2015

bbatsov Mar 22, 2015

bbatsov commented Mar 22, 2015

bbatsov commented Mar 22, 2015

EricGebhart commented Mar 23, 2015

EricGebhart commented Mar 23, 2015

bbatsov Mar 28, 2015

bbatsov commented Mar 28, 2015

expez commented Mar 28, 2015

bbatsov commented Mar 28, 2015

bbatsov commented Mar 28, 2015

expez commented Mar 28, 2015

vspinu commented Mar 28, 2015

expez commented Mar 28, 2015

EricGebhart commented Mar 29, 2015

bbatsov commented Mar 30, 2015

EricGebhart commented Mar 30, 2015

EricGebhart commented Mar 30, 2015

bbatsov commented Mar 31, 2015

EricGebhart commented Mar 31, 2015

EricGebhart commented Mar 31, 2015

bbatsov commented Mar 31, 2015

EricGebhart commented Mar 31, 2015

expez commented Apr 1, 2015

bbatsov Apr 2, 2015

bbatsov commented Apr 2, 2015

EricGebhart commented Apr 2, 2015

bbatsov commented Apr 2, 2015

[#1032] Combine jump-to-var and jump-to-resource into one function. #1036

[#1032] Combine jump-to-var and jump-to-resource into one function. #1036

Conversation

EricGebhart commented Mar 20, 2015

bbatsov Mar 22, 2015

Choose a reason for hiding this comment

bbatsov commented Mar 22, 2015

bbatsov commented Mar 22, 2015

EricGebhart commented Mar 23, 2015

EricGebhart commented Mar 23, 2015

bbatsov Mar 28, 2015

Choose a reason for hiding this comment

bbatsov commented Mar 28, 2015

expez commented Mar 28, 2015

bbatsov commented Mar 28, 2015

bbatsov commented Mar 28, 2015

expez commented Mar 28, 2015

vspinu commented Mar 28, 2015

expez commented Mar 28, 2015

EricGebhart commented Mar 29, 2015

bbatsov commented Mar 30, 2015

EricGebhart commented Mar 30, 2015

EricGebhart commented Mar 30, 2015

bbatsov commented Mar 31, 2015

EricGebhart commented Mar 31, 2015

EricGebhart commented Mar 31, 2015

bbatsov commented Mar 31, 2015

EricGebhart commented Mar 31, 2015

expez commented Apr 1, 2015

bbatsov Apr 2, 2015

Choose a reason for hiding this comment

bbatsov commented Apr 2, 2015

EricGebhart commented Apr 2, 2015

bbatsov commented Apr 2, 2015