Skip to content

Add cider-history command, based on browse-kill-ring #1916

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

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

johnv02139
Copy link
Contributor

#1860

Add "cider-history", based on functionality provided by the emacs "browse-kill-ring" package.

Lots of documentation is available within the commit, specifically in miscellaneous_features.md.

@bbatsov
Copy link
Member

bbatsov commented Jan 11, 2017

Apart from my small remarks, the PR looks pretty good.

@@ -8,6 +8,7 @@
* Add an option `cider-inspector-fill-frame` to control whether the cider inspector window fills its frame.
* [#1893](https://github.com/clojure-emacs/cider/issues/1893): Add negative prefix argument to `cider-refresh` to inhibit invoking of cider-refresh-functions
* [#1776](https://github.com/clojure-emacs/cider/issues/1776): Add new customization variable `cider-test-defining-forms` allowing new test defining forms to be recognized.
* [#1860](https://github.com/clojure-emacs/cider/issues/1860): Add a command `history` to browse the REPL command history and insert elements from it into the REPL buffer.
Copy link
Member

Choose a reason for hiding this comment

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

cider-repl-history

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the actual buffer-local-var is called cider-repl-input-history. Do you want me to refer to that, specifically?

Copy link
Member

Choose a reason for hiding this comment

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

I meant that actual command is going to be named cider-repl-history in the end. Or were you referring here to the REPL shortcut command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, when I wrote that, I was thinking of the REPL shortcut command -- the thing you access by hitting comma. I guess it makes more sense to refer to the emacs function. I'll change it.


;; This file is not part of GNU Emacs.

;; Based heavily on browse-kill-ring:
Copy link
Member

Choose a reason for hiding this comment

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

This attribution line is enough. Drop the copyright snippet that follows it.

@@ -0,0 +1,749 @@
;;; cider-history.el --- REPL input history browser
Copy link
Member

Choose a reason for hiding this comment

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

Let's name the file and the command cider-repl-history.

Copy link
Contributor Author

@johnv02139 johnv02139 Jan 11, 2017

Choose a reason for hiding this comment

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

What about the prefix?? You want all the functions named cider-repl-history-foo?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the prefix should be changed as well. cider-history is simply too generic.

(overlay-put cider-history-preview-overlay
'invisible t)))))

;; UNUSED
Copy link
Member

Choose a reason for hiding this comment

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

Let's just remove it if it's unused.


(defun cider-history-highlight-inserted (start end)
"Insert the text between START and END."
;; (when cider-history-highlight-inserted-item (cider-history-adjust-insert))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a reference to the function above, which as I said is now unnecessary. I left it there in case anyone had a weird setup or wanted to try with an old emacs, so it would be as easy as possible for them to reinstate this, instead of re-writing it from scratch. But I can yank it all.

Copy link
Member

Choose a reason for hiding this comment

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

Drop it. We don't care at all about supporting old Emacsen.

"Display items in the CIDER command history in another buffer."
(interactive)
(when (eq major-mode 'cider-history-mode)
(error "Already viewing the CIDER command history"))
Copy link
Member

Choose a reason for hiding this comment

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

error -> user-error


When "highlight-inserted" is turned on, you can customize the face of the
inserted text with the variable `cider-history-inserted-item-face`.

Copy link
Member

Choose a reason for hiding this comment

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

Kill this extra blank line.

@@ -233,3 +233,6 @@ section of your Leiningen project's configuration.

Note that the history is written to the file when you kill the REPL
buffer (which includes invoking `cider-quit`) or you quit Emacs.

There is a facility to browse the REPL history; see `REPL input history browser`
Copy link
Member

Choose a reason for hiding this comment

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

You can directly link to this section using page#section-name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I guess these are extremely general questions, but I don't find any answers by Googling, so:
(1) what's the mapping from the text of the headline ("REPL history browser") and the section name? I tried "#repl-history-browser" but it didn't seem to work quite right. Is there a way to give an explicit name?
(2) how can I test this, not just the linking, but any markdown I create? Do I have to push it to github and load it from there?

Copy link
Member

Choose a reason for hiding this comment

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

and when you find the history item you were looking for, you can insert it from
the history buffer into your REPL buffer.

![History Browser](images/history_browser.png)
Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot to add this image file.

@johnv02139 johnv02139 force-pushed the repl-history branch 3 times, most recently from e5181fe to 5831938 Compare January 11, 2017 08:46
@bbatsov bbatsov merged commit 3a1bc62 into clojure-emacs:master Jan 11, 2017
@bbatsov
Copy link
Member

bbatsov commented Jan 11, 2017

Great work! 👍

@johnv02139 johnv02139 deleted the repl-history branch January 11, 2017 17:56
@expez
Copy link
Member

expez commented Jan 16, 2017

Incredibly hot stuff @johnv02139!

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.

3 participants