Skip to content

Commit

Permalink
Use mnemonic keybindings for test commands
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeffvalk
jeffvalk committed Feb 4, 2016
1 parent 17b88cc commit 5682631
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 21 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -24,6 +24,7 @@ and try to associate the created connection with this project automatically.
* Improve stacktrace presentation of compiler errors (readability, DWIM point positioning).
* [#1458](https://github.com/clojure-emacs/cider/issues/1458): Separate nREPL messages by connections instead of by sessions.
* [#1226](https://github.com/clojure-emacs/cider/issues/1226): Enable running of all loaded and all project tests.
* Give test commands their own keybinding prefix. Use single-key mnemonics for these commands.

### Bugs fixed

Expand Down
20 changes: 10 additions & 10 deletions README.md
View file
Expand Up @@ -404,11 +404,11 @@ Keyboard shortcut | Description
<kbd>C-c M-t v</kbd> | Toggle var tracing.
<kbd>C-c M-t n</kbd> | Toggle namespace tracing.
<kbd>C-c C-u</kbd> | Undefine a symbol. If invoked with a prefix argument, or no symbol is found at point, prompt for a symbol.
<kbd>C-c , ,</kbd> | Run tests for namespace.
<kbd>C-c , <</kbd> | Run tests for all loaded namespaces.
<kbd>C-c , M-<</kbd> | Run tests for all project namespaces. This loads the additional namespaces.
<kbd>C-c , C-,</kbd> | Re-run test failures/errors.
<kbd>C-c , M-,</kbd> | Run test at point.
<kbd>C-c t t</kbd> | Run test at point.
<kbd>C-c t n</kbd> | Run tests for current namespace.
<kbd>C-c t l</kbd> | Run tests for all loaded namespaces.
<kbd>C-c t p</kbd> | Run tests for all project namespaces. This loads the additional namespaces.
<kbd>C-c t r</kbd> | Re-run test failures/errors.
<kbd>C-c C-t</kbd> | Show the test report buffer.
<kbd>M-.</kbd> | Jump to the definition of a symbol. If invoked with a prefix argument, or no symbol is found at point, prompt for a symbol.
<kbd>C-c M-.</kbd> | Jump to the resource referenced by the string at point.
Expand Down Expand Up @@ -740,11 +740,11 @@ additional functionality at your disposal.

Keyboard shortcut | Description
--------------------------------|-------------------------------
<kbd>C-c , ,</kbd> | Run tests for namespace.
<kbd>C-c , <</kbd> | Run tests for all loaded namespaces.
<kbd>C-c , M-<</kbd> | Run tests for all project namespaces. This loads the additional namespaces.
<kbd>C-c , C-,</kbd> | Re-run test failures/errors.
<kbd>C-c , M-,</kbd> | Run test at point.
<kbd>C-c t t</kbd> | Run test at point.
<kbd>C-c t n</kbd> | Run tests for current namespace.
<kbd>C-c t l</kbd> | Run tests for all loaded namespaces.
<kbd>C-c t p</kbd> | Run tests for all project namespaces. This loads the additional namespaces.
<kbd>C-c t r</kbd> | Re-run test failures/errors.
<kbd>M-p</kbd> | Move point to previous test.
<kbd>M-n</kbd> | Move point to next test.
<kbd>t</kbd> and <kbd>M-.</kbd> | Jump to test definition.
Expand Down
4 changes: 2 additions & 2 deletions cider-mode.el
View file
Expand Up @@ -206,7 +206,7 @@ Returns to the buffer in which the command was invoked."
(define-key map (kbd "C-c C-k") #'cider-load-buffer)
(define-key map (kbd "C-c C-l") #'cider-load-file)
(define-key map (kbd "C-c C-b") #'cider-interrupt)
(define-key map (kbd "C-c ,") #'cider-test-commands-map)
(define-key map (kbd "C-c t") #'cider-test-commands-map)

This comment has been minimized.

Sign in to view

Copy link
@bbatsov

bbatsov Feb 5, 2016

Member

We can't use such a prefix - http://www.gnu.org/software/emacs/manual/html_node/elisp/Key-Binding-Conventions.html (amusingly I've misled you yesterday - C-c , wasn't reserved for the users; no idea who came up with this exception).

Btw, turns out our old prefix was actually allowed.

This comment has been minimized.

Sign in to view

Copy link
@bbatsov

bbatsov Feb 5, 2016

Member

Seems it'd be best to use C-c C-t as the prefix.

This comment has been minimized.

Sign in to view

Copy link
@Malabarba

Malabarba Feb 5, 2016

Member

I think it would be best to keep the old prefix (C-c ,) in addition to the new. Let's minimize friction for the users.

This comment has been minimized.

Sign in to view

Copy link
@bbatsov

bbatsov Feb 5, 2016

Member

Fair enough.
(define-key map (kbd "C-c C-t") #'cider-test-show-report)

This comment has been minimized.

Sign in to view

Copy link
@bbatsov

bbatsov Feb 5, 2016

Member

This should probably be in the test map as well.
(define-key map (kbd "C-c M-s") #'cider-selector)
(define-key map (kbd "C-c M-r") #'cider-rotate-default-connection)
Expand Down Expand Up @@ -242,7 +242,7 @@ Returns to the buffer in which the command was invoked."
["Go back" cider-pop-back])
("Test"
["Run test" cider-test-run-test]
["Run namespace tests" cider-test-run-tests]
["Run namespace tests" cider-test-run-ns-tests]
["Run all loaded tests" cider-test-run-loaded-tests]
["Run all project tests" cider-test-run-project-tests]
["Rerun failed/erring tests" cider-test-rerun-tests]
Expand Down
18 changes: 9 additions & 9 deletions cider-test.el
View file
Expand Up @@ -118,16 +118,16 @@

(defvar cider-test-commands-map
(let ((map (define-prefix-command 'cider-test-commands-map)))
(define-key map (kbd ",") #'cider-test-run-tests)
(define-key map (kbd "M-,") #'cider-test-run-test)
(define-key map (kbd "C-,") #'cider-test-rerun-tests)
(define-key map (kbd "<") #'cider-test-run-loaded-tests)
(define-key map (kbd "M-<") #'cider-test-run-project-tests)
(define-key map (kbd "r") #'cider-test-rerun-tests)

This comment has been minimized.

Sign in to view

Copy link
@bbatsov

bbatsov Feb 5, 2016

Member

You should also double all commands to have a C-something version (see the doc map for reference). Some prefer not having to stop pressing C for the duration of the entire keybinding.
(define-key map (kbd "t") #'cider-test-run-test)
(define-key map (kbd "n") #'cider-test-run-ns-tests)
(define-key map (kbd "l") #'cider-test-run-loaded-tests)
(define-key map (kbd "p") #'cider-test-run-project-tests)
map))

(defvar cider-test-report-mode-map
(let ((map (make-sparse-keymap)))
(define-key map (kbd "C-c ,") #'cider-test-commands-map)
(define-key map (kbd "C-c t") #'cider-test-commands-map)
(define-key map (kbd "M-p") #'cider-test-previous-result)
(define-key map (kbd "M-n") #'cider-test-next-result)
(define-key map (kbd "M-.") #'cider-test-jump)
Expand Down Expand Up @@ -383,7 +383,7 @@ With the actual value, the outermost '(not ...)' s-expression is removed."
(cond ((stringp ns) ns)
((eq :non-passing ns) "failing")
((eq :loaded ns) "all loaded")
((eq :project ns) "all")))
((eq :project ns) "all project")))
(unless (stringp ns) " namespaces"))))

(defun cider-test-echo-summary (summary results)
Expand Down Expand Up @@ -569,8 +569,8 @@ are highlighted."
(interactive)
(cider-test-execute :project))

(defun cider-test-run-tests (suppress-inference)
"Run all tests for the current Clojure source or test report context.
(defun cider-test-run-ns-tests (suppress-inference)
"Run all tests for the current Clojure namespace context.
With a prefix arg SUPPRESS-INFERENCE it will try to run the tests in the
current ns."
Expand Down

8 comments on commit 5682631

@jeffvalk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbatsov @Malabarba Since we're going to break the old keybindings, I think we should pick one convention. Either:

C-t , t
C-t , r

or

C-c C-t C-t
C-c C-t C-r

My preference is the former.

@bbatsov
Copy link
Member

@bbatsov bbatsov commented on 5682631 Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C-t , t
C-t , r

I guess you meant to write C-c , t/r. Frankly, I don't see the harm in having two prefixes for the next release and I definitely prefer having something more meaningful as C-c C-t as the prefix. One of my big goals is to make cider more approachable to newcomers, so we should try to be more consistent with our keybindings.

And it's also important to note that power users can always tweak those. I've done something similar for my Projectile project - scroll to the instructions right after the keybindings table here https://github.com/bbatsov/projectile#interactive-commands

@bbatsov
Copy link
Member

@bbatsov bbatsov commented on 5682631 Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, you should also take a look at the failure here https://travis-ci.org/clojure-emacs/cider/jobs/107199514

@jeffvalk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbatsov Yep, pardon my typo! I'm all for consistency; that's why I'm raising an eyebrow at introducing multiple prefixes for the same thing. That said, your point about being approachable is well taken.

If we're to have both C-c C-t and C-c , as prefixes, which one would you like displayed in menus?

(Also, I will look at that test failure. Good tip.)

@bbatsov
Copy link
Member

@bbatsov bbatsov commented on 5682631 Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're to have both C-c C-t and C-c , as prefixes, which one would you like displayed in menus?

C-c C-t.

@Malabarba
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jeffvalk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated in 3a4cdb3.

@jeffvalk
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make that 813928a...to fix the build error mentioned above.

Please sign in to comment.