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 eval-top-level-to-comment command. #1536

Merged
merged 1 commit into from Feb 2, 2016

Conversation

phillord
Copy link
Contributor

As discussed ages ago!

(interactive)
(let ((defun-at-point (cider-defun-at-point))
(point-at-defun (cider-defun-at-point-start-pos)))
(goto-char point-at-defun)
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 we should wrap all this in a save-excursion I don't like it when commands move point when they don't have to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried this. If you surround the whole thing in save-excursion then it prints

results-of-eval;; =>

rather than

;; => results of-eval

Why? Don't know. I am guessing that the call-back cider-eval-print-handler happens asynchronously? save-excursion has already emptied?

@Malabarba
Copy link
Member

This looks a little roundabout. Why does it need to goto-char and insert two separate times?

@@ -1050,6 +1050,22 @@ If invoked with a PREFIX argument, print the result in the current buffer."
(backward-kill-sexp)
(cider-interactive-eval last-sexp (cider-eval-print-handler))))

(defun cider-eval-top-level-to-comment ()
"Evaluate the \"top-level\" form at point and insert result as pre-comment."
Copy link
Member

Choose a reason for hiding this comment

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

Why a pre-comment? Maybe there should be some option to choose either a pre or a post comment?

@Malabarba
Copy link
Member

Also, is there a reason to insert above? When you see code samples the results comment is usually below the snippet.

@expez
Copy link
Member

expez commented Jan 31, 2016

I agree that having the result above the form is non-standard. In many examples you also see a marker signifying evaluation like:

(+ 1 1)
;;=> 2

I prefer this style, but I'm probably not going to use this feature a lot so I'm good either way.

@phillord
Copy link
Contributor Author

Why comments before? Because comments always come before the thing to which they refer. That's how I've always seen it anyway.

@phillord
Copy link
Contributor Author

Why the two goto-chars --- I don't understand the semantics of the functions I am using at the moment, as I said above -- surround the whole thing in save-excursion and you get a different result.

In this case, we move the form forward first, then add the eval result (otherwise, we loose the end of the eval result.

There are other ways, of course.

@phillord
Copy link
Contributor Author

Oh, yeah, wanted to ask. At the moment, this misbehaves when the eval errors. cider-eval-last-sexp-and-replace calls nrepl-sync-request:eval first, and then cider-interactive-eval. Does this not eval the form twice?

@phillord
Copy link
Contributor Author

All changed.

All the insertion logic is in the callback now. point is preserved, errors are handled, comments after or before on prefix arg and generally everything is cool and froody.

@@ -664,7 +664,7 @@ can be used to display the evaluation result."
(funcall nrepl-err-handler)))))

(defun cider-eval-print-handler (&optional buffer)
"Make a handler for evaluating and printing result in BUFFER."
"Make a handler for evaluating and printing result in BUFFER at LOCATION."
Copy link
Member

Choose a reason for hiding this comment

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

You changed this docstring, but I think you meant to add this to the function below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear. I keep forgetting to check the overall diff.

@@ -1050,6 +1067,21 @@ If invoked with a PREFIX argument, print the result in the current buffer."
(backward-kill-sexp)
(cider-interactive-eval last-sexp (cider-eval-print-handler))))

(defun cider-eval-defun-to-comment (loc)
Copy link
Member

Choose a reason for hiding this comment

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

LOC should appear somewhere in the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

This is still not addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be this evening now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Complete now

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2016

You'll have to rebase on top of the current master. Please drop the . from the commit message's title.

@phillord
Copy link
Contributor Author

phillord commented Feb 1, 2016

All done. Travis failure seems to be an existing problem with master.

@@ -678,6 +678,23 @@ can be used to display the evaluation result."
(cider-emit-interactive-eval-err-output err))
'()))

(defun cider-eval-print-with-comment-handler (buffer location comment)
Copy link
Member

Choose a reason for hiding this comment

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

comment -> comment-prefix

bbatsov added a commit that referenced this pull request Feb 2, 2016
Add `cider-eval-defun-to-comment` command.
@bbatsov bbatsov merged commit 5684cff into clojure-emacs:master Feb 2, 2016
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.

None yet

4 participants