Implement cider-pprint-eval-last-sexp-to-comment #2111

gonewest818 · 2017-11-10T22:19:53Z

This function has similar functionality to the other pprint interactions;
however this variant inserts the result of the evaluation directly in the
current buffer.

The comment prefix used in the output is user-configurable, and for consistency
we also use same prefix in cider-eval-defun-to-comment.

By default the output will be commented like so

(for [i (range 1 10)]
  (range i))
;; => ((0)
;;     (0 1)
;;     (0 1 2)
;;     (0 1 2 3)
;;     (0 1 2 3 4)
;;     (0 1 2 3 4 5)
;;     (0 1 2 3 4 5 6)
;;     (0 1 2 3 4 5 6 7)
;;     (0 1 2 3 4 5 6 7 8))

but by customizing the cider-pprint-comment-* variables you can also get output like this

#_((0)
   (0 1)
   (0 1 2)
   (0 1 2 3)
   (0 1 2 3 4)
   (0 1 2 3 4 5)
   (0 1 2 3 4 5 6)
   (0 1 2 3 4 5 6 7)
   (0 1 2 3 4 5 6 7 8))

or even

(comment
  ((0)
   (0 1)
   (0 1 2)
   (0 1 2 3)
   (0 1 2 3 4)
   (0 1 2 3 4 5)
   (0 1 2 3 4 5 6)
   (0 1 2 3 4 5 6 7)
   (0 1 2 3 4 5 6 7 8))
)

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (make test)
The new code is not generating bytecode or M-x checkdoc warnings
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the readme (if adding/changing user-visible functionality)
You've updated the refcard (if you made changes to the commands listed there)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

gonewest818 · 2017-11-10T23:07:57Z

AFAICT, some of those travis errors look like transient network issues on their end (e.g. "Opening TLS connection to ‘melpa.org’...failed"), while the other errors refer to a file I didn't change (cider-stacktrace.el).

bbatsov · 2017-11-26T10:04:20Z

AFAICT, some of those travis errors look like transient network issues on their end (e.g. "Opening TLS connection to ‘melpa.org’...failed"),

Or something to do with changes in Emacs 26. We have to investigate this.

bbatsov · 2017-11-26T10:04:52Z

cider-interaction.el

+  "The prefix to insert before the first line of pprint output."
+  :type 'string
+  :group 'cider
+  :package-version '(cider . "0.15.2"))


The version should 0.16 everywhere. There won't be 0.15.2.

Ok, I’ll fix that.

gonewest818 · 2017-11-27T06:22:09Z

With respect to the TLS error in Travis... I think the difference is emacs versions <=25.2 fall back to s_client when gnutls-cli fails, but emacs 26 does not (see discussion in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=766397 and commit
emacs-mirror/emacs@6e45de6)

Even in the failed Travis jobs, there's a point where evm successfully bootstraps itself using emacs-24.3-travis: https://travis-ci.org/clojure-emacs/cider/jobs/307715504#L485-L492
where you see it's the s_client attempt that actually succeeds.

The build failure occurs later, as cask is installing dependencies for emacs-git-snapshot-travis:
https://travis-ci.org/clojure-emacs/cider/jobs/307715504#L879-L885
and you can see there is no s_client fallback attempt.

So I'm not sure why gnutls-cli is failing in Travis, perhaps the cacert isn't valid (or isn't where it's supposed to be)? Or perhaps something else.

bbatsov · 2017-12-01T06:56:21Z

Thanks for looking into this. I guess the easiest think we can do is to switch to just http to avoid those errors, although I don't like this that much.

Please mention your changes in the changelog and in the relevant parts of the manual.

bbatsov · 2017-12-01T06:57:00Z

You should probably add the new command to some of the mode menus as well, otherwise no one will find it.

gonewest818 · 2017-12-01T20:24:20Z

Ok, will do. Want me to squash commits when all is said and done?

…

On Nov 30, 2017, at 10:57 PM, Bozhidar Batsov ***@***.***> wrote: You should probably add the new command to some of the mode menus as well, otherwise no one will find it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

bbatsov · 2017-12-01T21:22:12Z

@gonewest818 Yep, that'd be best.

gonewest818 · 2017-12-04T19:39:13Z

I made this feature invokable with a prefix argument on cider-pprint-eval-last-sexp. In other words, C-u C-c C-p pretty-prints the last sexp and inserts the resulting text as as comment in the current buffer.

I updated the changelog and the docs to indicate this is the case. I didn't change README.md, as it seems more like a landing page with links off to those other docs I already changed.

bbatsov · 2017-12-05T06:31:02Z

cider-interaction.el

                            bounds)))

+(defun cider-pprint-eval-last-sexp-to-comment (loc)


Guess you should make loc optional. And maybe name it insert-result-before or something more meaningful.

That matches the implementation of cider-eval-defun-to-comment so I'l fix both.

bbatsov · 2017-12-05T06:35:37Z

cider-interaction.el

+(defun cider-pprint-eval-last-sexp-to-comment (loc)
+  "Evaluate the last sexp and insert result as comment at LOC.
+
+With a prefix arg, LOC, insert before the form, otherwise afterwards."


Maybe you should mention here the configuration options related to this command.

bbatsov · 2017-12-05T06:37:23Z

cider-interaction.el

-  "Evaluate the sexp preceding point and pprint its value in a popup buffer."
-  (interactive)
-  (cider--pprint-eval-form (cider-last-sexp 'bounds)))
+(defun cider-pprint-eval-last-sexp (&optional prefix)


Normally I avoid the name prefix and go for something like print-in-current-buffer (or something).

gonewest818 · 2017-12-07T04:53:19Z

ok @bbatsov, code review comments are addressed and I squashed the commits again.

xiongtx · 2017-12-07T08:20:04Z

cider-interaction.el

-                             (current-buffer) insertion-point ";; => ")
+                             (current-buffer)
+                             insertion-point
+                             cider-pprint-comment-prefix)


This introduces a customizable variable called cider-pprint-comment-prefix, but it's also used in cider-eval-defun-to-comment.

Seems that the variables can be used in comment functions in general, not just the pprint variants, so we can drop the pprint from the names:

cider-comment-prefix

cider-comment-continued-prefix

cider-comment-postfix

yes, that's a fair point. I'll do that.

done. see commit.

xiongtx · 2017-12-07T08:29:47Z

cider-interaction.el

                            bounds)))

+(defun cider-pprint-eval-last-sexp-to-comment (&optional insert-before)


Currently, CIDER has only has cider-eval-defun-to-comment. Seems asymmetric to add cider-pprint-eval-last-sexp-to-comment instead of cider-pprint-eval-defun-to-comment. Could we implement the latter instead?

I can, but symmetry is arguably a larger issue than this, considering:

cider-eval-sexp-at-point

cider-eval-last-sexp

cider-eval-print-last-sexp

cider-eval-last-sexp-to-repl

cider-eval-last-sexp-to-comment (Created cider-eval-last-sexp-to-comment #2101)

cider-eval-defun-at-point

cider-eval-defun-to-comment

cider-pprint-eval-last-sexp

cider-pprint-eval-last-sexp-to-repl

cider-pprint-eval-last-sexp-to-comment (Implement cider-pprint-eval-last-sexp-to-comment #2111)

cider-pprint-eval-defun-at-point

OK, I didn't see #2101. But we'd still have no cider-pprint-eval-defun-to-comment 🤷‍♂️. Maybe add it in the next PR?

Let me take a look. I doubt the defun case is really all that different from what I have.

But, for the record there's a lot of overlapping functionality in this set. Everything amounts to "eval {some selected form} and send result to {some destination} {with/without pprint}". It seems clear to me this could be more aggressively refactored such that you don't actually have to write distinct functions for each of the combinatorial possibilities. I'm not proposing to do that now, mind you, that's too much scope for what was supposed to be an incremental feature.

done. see the latest commit.

gonewest818 · 2017-12-08T00:42:07Z

I found a bug in this implementation but I can't figure out how to resolve it.

If I invoke these new functions by name, e.g. M-x cider-pprint-eval-last-sexp-to-comment the function executes and silently exits without seeming to modify the buffer. It's the next character I type that causes the result to appear in the buffer.

Whereas, if I invoke the same function through its keybinding C-u C-c C-p the function executes and the buffer updates immediately. Any hints why?

bbatsov · 2017-12-08T08:46:09Z

cider-interaction.el

+If invoked with OUTPUT-TO-CURRENT-BUFFER, insert as comment in the current buffer."
+  (interactive "P")
+  (if output-to-current-buffer
+      (cider-pprint-eval-last-sexp-to-comment nil)


You don't have to pass nil here - now it's optional, which means the param is going to be nil by default.

right. thanks!

bbatsov · 2017-12-08T16:31:09Z

Whereas, if I invoke the same function through its keybinding C-u C-c C-p the function executes and the buffer updates immediately. Any hints why?

See the debugging section of the manual. Using the Elisp debugger is the best way to see what's going wrong.

gonewest818 · 2017-12-08T20:19:10Z

I couldn't reproduce the problem in the debugger, but I managed to find it anyway. In emacs 25.1 and newer (simple.el, in the function execute-extended-command) there's a new UX feature that suggests faster ways to invoke a command:

You can run the function 'cider-pprint-eval-defun-to-comment' with M-x -p-d-t RET

https://github.com/emacs-mirror/emacs/blob/emacs-25/lisp/simple.el#L1722-L1730

A comment in the code mentions a caveat that the search for a shorter-but-unique string "can be slow". And so it seems, at least in this particular case. If the function has a keybinding, emacs suggests the available keybinding rather than trying to find a shorter string. And that is fast.

If this bug is really objectionable to a user, one workaround would be (setq suggest-key-bindings nil) and turn off that suggestion feature. But by far the typical usage will be to invoke this functionality via the provided keybindings so I'm inclined to just document this as a known issue and move on. Objections?

bbatsov · 2017-12-09T10:53:56Z

Ah, yeah. Now I remembered about that feature. However, I don't recall ever using it in practice. I guess I don't use M-x very often. :-)

bbatsov · 2017-12-09T10:56:31Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### New Features

 * [#2082](https://github.com/clojure-emacs/cider/pull/2082), [cider-nrepl#440](https://github.com/clojure-emacs/cider-nrepl/pull/440): Add specialized stacktraces for clojure.spec assertions.
+* [#2111](https://github.com/clojure-emacs/cider/pull/2111): Add `cider-pprint-eval-last-sexp-to-comment` and `cider-pprint-eval-defun-to-comment`


This sentence should end with a ..

bbatsov · 2017-12-09T10:57:04Z

Seems we're finally ready to merge then. Address my tiny remark and update your commit message in light of the final state of the PR.

…-comment These functions have similar functionality to the other pprint interactions; however now we can also insert the result of evaluation directly in the current buffer. The comment prefix(es) used in output is now user-configurable, and for consistency, we also use same prefix in cider-eval-defun-to-comment.

gonewest818 · 2017-12-09T17:55:52Z

Done. Thanks for your helpful (and patient) feedback.

bbatsov · 2017-12-10T00:02:45Z

🎉

You're welcome! Looking forward to more contributions from you down the road! 😉

bbatsov reviewed Nov 26, 2017

View reviewed changes

gonewest818 mentioned this pull request Dec 4, 2017

Travis CI errors with EMACS_BINARY=emacs-git-snapshot-travis #2120

Closed

gonewest818 force-pushed the cider-pprint-eval-last-sexp-to-comment branch from 192e918 to 3835eca Compare December 4, 2017 23:33

bbatsov reviewed Dec 5, 2017

View reviewed changes

gonewest818 force-pushed the cider-pprint-eval-last-sexp-to-comment branch from 1decb7e to 67493a5 Compare December 7, 2017 04:51

xiongtx reviewed Dec 7, 2017

View reviewed changes

bbatsov reviewed Dec 8, 2017

View reviewed changes

bbatsov reviewed Dec 9, 2017

View reviewed changes

bbatsov mentioned this pull request Dec 9, 2017

Created cider-eval-last-sexp-to-comment #2101

Closed

gonewest818 force-pushed the cider-pprint-eval-last-sexp-to-comment branch from 4d98630 to 0bce19a Compare December 9, 2017 17:46

gonewest818 force-pushed the cider-pprint-eval-last-sexp-to-comment branch from 0bce19a to 89af027 Compare December 9, 2017 17:54

bbatsov merged commit 4524100 into clojure-emacs:master Dec 10, 2017

gonewest818 deleted the cider-pprint-eval-last-sexp-to-comment branch February 1, 2018 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cider-pprint-eval-last-sexp-to-comment #2111

Implement cider-pprint-eval-last-sexp-to-comment #2111

gonewest818 commented Nov 10, 2017 •

edited

Loading

gonewest818 commented Nov 10, 2017

bbatsov commented Nov 26, 2017

bbatsov Nov 26, 2017

gonewest818 Nov 26, 2017

gonewest818 commented Nov 27, 2017 •

edited

Loading

bbatsov commented Dec 1, 2017

bbatsov commented Dec 1, 2017

gonewest818 commented Dec 1, 2017 via email

bbatsov commented Dec 1, 2017

gonewest818 commented Dec 4, 2017 •

edited

Loading

bbatsov Dec 5, 2017

gonewest818 Dec 7, 2017

bbatsov Dec 5, 2017

bbatsov Dec 5, 2017

gonewest818 commented Dec 7, 2017

xiongtx Dec 7, 2017

gonewest818 Dec 7, 2017

gonewest818 Dec 8, 2017

xiongtx Dec 7, 2017

gonewest818 Dec 7, 2017

xiongtx Dec 7, 2017

gonewest818 Dec 7, 2017 •

edited

Loading

gonewest818 Dec 8, 2017

gonewest818 commented Dec 8, 2017

bbatsov Dec 8, 2017

gonewest818 Dec 8, 2017

bbatsov commented Dec 8, 2017

gonewest818 commented Dec 8, 2017 •

edited

Loading

bbatsov commented Dec 9, 2017

bbatsov Dec 9, 2017

gonewest818 Dec 9, 2017

bbatsov commented Dec 9, 2017

gonewest818 commented Dec 9, 2017

bbatsov commented Dec 10, 2017

		bounds)))

		(defun cider-pprint-eval-last-sexp-to-comment (loc)

Implement cider-pprint-eval-last-sexp-to-comment #2111

Implement cider-pprint-eval-last-sexp-to-comment #2111

Conversation

gonewest818 commented Nov 10, 2017 • edited Loading

gonewest818 commented Nov 10, 2017

bbatsov commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonewest818 commented Nov 27, 2017 • edited Loading

bbatsov commented Dec 1, 2017

bbatsov commented Dec 1, 2017

gonewest818 commented Dec 1, 2017 via email

bbatsov commented Dec 1, 2017

gonewest818 commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonewest818 commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonewest818 Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonewest818 commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Dec 8, 2017

gonewest818 commented Dec 8, 2017 • edited Loading

bbatsov commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Dec 9, 2017

gonewest818 commented Dec 9, 2017

bbatsov commented Dec 10, 2017

gonewest818 commented Nov 10, 2017 •

edited

Loading

gonewest818 commented Nov 27, 2017 •

edited

Loading

gonewest818 commented Dec 4, 2017 •

edited

Loading

gonewest818 Dec 7, 2017 •

edited

Loading

gonewest818 commented Dec 8, 2017 •

edited

Loading