Skip to content
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

Add default keybindings for rename-ns-alias and add-arity refactorings, and refactor clojure-unwind #538

Merged
merged 4 commits into from
Jul 16, 2019

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Jul 12, 2019

I placed these suggested bindings under the clojure-refactor-map and added them to the menubar:
C-c C-r n r -> clojure-rename-ns-alias
C-c C-r a -> clojure-add-arity

The a keybinding was taken by clojure-unwind-all which didn't made much sense, so I took the liberty of changing it to U as a "stronger" version of the existing u -> clojure-unwind binding.


Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our [contribution guidelines][1].
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

@bbatsov
Copy link
Member

bbatsov commented Jul 12, 2019

The a keybinding was taken by clojure-unwind-all which didn't made much sense, so I took the liberty of changing it to U as a "stronger" version of the existing u -> clojure-unwind binding.

Uppercase bindings are both hard to press and a bit confusing for the end users, so I generally avoid them. Probably in this case we can fold a few of the *-all commands into the parent commands and make the invokable with a prefix arg.

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jul 12, 2019

Would you like that to be done in this PR? It sounds like quite a big breaking change and personally I prefer having separate commands for discoverability instead of using prefix args.

Another option in this case could be u w -> unwind and u a -> unwind-all, or u for one and C-u for the other, although that would somewhat break symmetry with the rest of the bindings.

@bbatsov
Copy link
Member

bbatsov commented Jul 12, 2019

Would you like that to be done in this PR? It sounds like quite a big breaking change and personally I prefer having separate commands for discoverability instead of using prefix args.

That makes sense under a certain point - when you start running out of meaningful keybindings you know you have to start compacting things and group them together. I'm not saying to remove the existing command - I'm just saying to remove its dedicated keybinding. I think such commands are a classic use case for a prefix key as technically this is just an extension of the core behaviour.

Changing the existing key is just as big of a breaking change as far as I'm concerned, so nothing really changes in the end.

Another option in this case could be u w -> unwind and u a -> unwind-all, or u for one and C-u for the other, although that would somewhat break symmetry with the rest of the bindings.

Extra prefixes should be utilized only to group multiple related commands. E.g. - if we had more commands that operated on function definitions it would make sense to put them under some shared prefix.

yuhan0 added 3 commits July 13, 2019 17:18
Numeric prefix arg unwinds N levels, and universal arg unwinds all.

clojure-unwind-all command is left as an alias for calling clojure-unwind with
universal argument.
Removed keybinding for clojure-unwind-all (use C-u universal arg with
clojure-unwind instead)
@yuhan0
Copy link
Contributor Author

yuhan0 commented Jul 13, 2019

Ok, I refactored clojure-unwind to take a numeric prefix / universal argument, and removed the dedicated key binding for clojure-unwind-all

@bbatsov
Copy link
Member

bbatsov commented Jul 14, 2019

The changes look great! Don't forget to update the changelog as well.

@yuhan0 yuhan0 changed the title Add default keybindings for rename-ns-alias and add-arity refactorings Add default keybindings for rename-ns-alias and add-arity refactorings, and refactor clojure-unwind Jul 16, 2019
@yuhan0
Copy link
Contributor Author

yuhan0 commented Jul 16, 2019

Done, and added a couple of tests as well :)

@bbatsov bbatsov merged commit 5a3c6ea into clojure-emacs:master Jul 16, 2019
@bbatsov
Copy link
Member

bbatsov commented Jul 16, 2019

Lovely! Thank you! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants