New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated ensime bindings to better fit with spacemacs #600

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@bjarkevad
Contributor

bjarkevad commented Feb 14, 2015

I've updated the bindings for ensime so they fit a bit better with spacemacs, but rather than following spacemacs convetions I've pretty much just translated ensime's default bindings from C-c C-{X} something to spc m {X} something.
I'm not sure if this is the best idea, but it makes switching from normal emacs + ensime to spacemacs + ensime easier.

@chrisbarrett

This comment has been minimized.

Contributor

chrisbarrett commented Feb 14, 2015

I like this. +1

"mro" 'ensime-refactor-organize-imports
"mrr" 'ensime-refactor-rename
"mrt" 'ensime-import-type-at-point

This comment has been minimized.

@syl20bnr

syl20bnr Feb 15, 2015

Owner

It is not in the conventions for now but the refactoring is planned to be on SPC m R instead of SPC m r.

"mbr" 'ensime-sbt-do-run
"mbs" 'ensime-sbt-switch
"mbt" 'ensime-sbt-do-test-quick

This comment has been minimized.

@syl20bnr

syl20bnr Feb 15, 2015

Owner

I like mb for sbt commands!

"mti" 'ensime-goto-impl
"mtt" 'ensime-goto-test

This comment has been minimized.

@syl20bnr

syl20bnr Feb 15, 2015

Owner

mt is reserved for tests.
At first sight these commands should go behind mg

This comment has been minimized.

@bjarkevad

bjarkevad Feb 16, 2015

Contributor

So ensime-goto-{test,impl} is used to jump between test and implementation files, so it's not a mismatch with spacemacs conventions if we want to keep them this way.

@syl20bnr

This comment has been minimized.

Owner

syl20bnr commented Feb 15, 2015

I will wait a bit before merging it because I have to read the doc of each command and maybe play a bit with them in order to make them match with what we have in the other layers.

Each new added tool is a great opportunity to discover new commands so this PR has a lot of value. Thank you !

@bjarkevad

This comment has been minimized.

Contributor

bjarkevad commented Feb 15, 2015

Thanks for the feedback!
As a sidenote, when I was reading the ensime docs I noticed that a lot of times only the keybinding was refered to, so it was hard to translate ensime bindings to spacemacs-convention bindings without making ensime docs hard to follow.

@@ -47,6 +47,78 @@ which require an initialization must be listed explicitly in the list.")
(kbd "n") 'forward-button
(kbd "N") 'backward-button)
(defun ensime-gen-and-reload()

This comment has been minimized.

@bjarkevad

bjarkevad Feb 16, 2015

Contributor

I've added a new function which generates an .ensime and reloads ensime, which is useful when the project changes. Sometimes it's also necessary to rm -r .ensime_cache in the project root to force ensime to notice changes to e.g. dependencies.

"mg" 'ensime-edit-definition
"m." 'ensime-gen-and-reload
"m," 'ensime

This comment has been minimized.

@bjarkevad

bjarkevad Feb 16, 2015

Contributor

Using , , and , . for ensime-server commands. Could probably be moved to some more mnemonic binding.

)
(evil-leader/set-key-for-mode 'scala-mode
"mg" 'ensime-edit-definition

This comment has been minimized.

@bjarkevad

bjarkevad Feb 16, 2015

Contributor

This is a temporary binding, but the function should be included in the bindings

@syl20bnr

This comment has been minimized.

Owner

syl20bnr commented Feb 18, 2015

Excellent PR ! 👍 ❤️
I adapted it to the conventions in this commit: 4be4abc

Cherry-picked into develop.

@syl20bnr syl20bnr closed this Feb 18, 2015

@syl20bnr

This comment has been minimized.

Owner

syl20bnr commented Feb 18, 2015

@bjarkevad, forgot to mention that I decided to put refactoring bindings under mr instead of mR since it does not make sense to use R which is harder to press.
I originally planned to put them under mR because ruby already uses mr for RoR bindings, but it makes little sense to shift all refactoring bindings on mR just for ruby.

CC @bru We have 2 solutions:

  1. move RoR bindings to another key that is easy to press with m (could be mn, mm etc...) but not mnemonic
  2. make an exception with ruby and put refactoring on mR

I would go with 1) to keep consistency with mr everywhere.

@bru

This comment has been minimized.

Contributor

bru commented Feb 18, 2015

Considered the fact that currently the ruby layer has 1 refactor command vs
a few dozen Rails commands, I would yield precedence to the latter.
That said, 'mm' could be a better prefix given: 1. the (very high)
frequency we need code navigation commands and 2. the fact the ',' shortcut
still doesn't work in all modes (so I end up always using the long form)
On Feb 18, 2015 2:41 PM, "Sylvain Benner" notifications@github.com wrote:

@bjarkevad https://github.com/bjarkevad, forgot to mention that I
decided to put refactoring bindings under mr instead of mR since it does
not make sense to use R which is harder to press.
I originally planned to put them under mR because ruby already uses mr
for RoR bindings, but it makes little sense to shift all refactoring
bindings on mR just for ruby.

CC @bru https://github.com/bru We have 2 solutions:

  1. move RoR bindings to another key that is easy to press with m (could be
    mn, mm etc...) but not mnemonic
  2. make an exception with ruby and put refactoring on mR

I would go with 1) to keep consistency with mr everywhere.


Reply to this email directly or view it on GitHub
#600 (comment).

@syl20bnr

This comment has been minimized.

Owner

syl20bnr commented Feb 18, 2015

, should work whenever there is SPC m for the current mode, if not it is a big bug :-(
Do you have examples where there are SPC m bindings available and , does not work ?

@bru

This comment has been minimized.

Contributor

bru commented Feb 18, 2015

Essentially any view in a rails project.
I thought that was happening because a views's major mode is HTML/web, but
the long form (SPC m ...) still works, so I'm a bit puzzled. Happy to help
you debugging this, of course.
On Feb 18, 2015 7:07 PM, "Sylvain Benner" notifications@github.com wrote:

, should work whenever there is SPC m for the current mode, if not it is
a big bug :-(
Do you have examples where there are available SPC m bindings available
and , does not work ?


Reply to this email directly or view it on GitHub
#600 (comment).

@syl20bnr

This comment has been minimized.

Owner

syl20bnr commented Feb 23, 2015

I never thought about this... this is a big limitation, SPC m is major-mode dependent. This is really bad :-(

Extending evil-leader to minor-modes can be useful then but we have to be extra-careful about using it.
What rule do you have to enable rails command as a minor mode in the views ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment