greghendershott / racket-mode

Emacs major and minor modes for Racket: edit, REPL, check-syntax, debug, profile, packages, and more.
https://www.racket-mode.com/
GNU General Public License v3.0
682 stars 93 forks source link

[QOL wish] Better `racket-send-last-sexp` =>`insert-last-sexp-to-REPL-and-eval` #607

Closed shenlebantongying closed 2 years ago

shenlebantongying commented 2 years ago

"racket-send-last-sexp" doesn't give any feedback on what outputted a result. insert-last-sexp-to-REPL-and-eval can be provided as an alternative eval.

One can easily produce similar or identical results with different sexp. I think this violated the design principle that action should be associated with feedback because feedback provides reassurance. A lack of feedback creates a feeling of lack of control, which can be unsettling. Also, when (definesomething, there is almost no feedback. One cannot know what was defined/redefined recently by a simple glimpse.

Minimum comparison: racket-send-last-sexp

Screen Shot 2022-03-27 at 5 55 54 AM

insert-last-sexp-to-REPL-and-eval

Screen Shot 2022-03-27 at 6 04 57 AM

Also the insert-last-sexp-to-REPL-and-eval implementated in Cider, Clojure's emacs mode.

Screen Shot 2022-03-27 at 6 22 51 AM
greghendershott commented 2 years ago

Under the hood this uses process-send-string, which does not copy the string into the buffer.

As a quick hack, this seems close to the behavior you want, assuming you use a C-u prefix before the command:

@@ -688,7 +688,7 @@ most recent `racket-mode' buffer, if any."

 ;;; send to REPL

-(defun racket--send-region-to-repl (start end)
+(defun racket--send-region-to-repl (start end &optional echo-p)
   "Internal function to send the region to the Racket REPL.

 Before sending the region, calls `racket-repl' and
@@ -704,11 +704,15 @@ Afterwards displays the buffer in some window."
   (let ((source-buffer (current-buffer)))
     (racket-repl t)
     (racket--repl-forget-errors)
-    (let ((proc (get-buffer-process racket-repl-buffer-name)))
+    (let ((proc (get-buffer-process racket-repl-buffer-name))
+          (str  (if echo-p (buffer-substring start end) "")))
       (with-racket-repl-buffer
         (save-excursion
           (goto-char (process-mark proc))
           (insert ?\n)
+          (when echo-p
+            (insert str)
+            (insert "\n;; =>\n"))
           (set-marker (process-mark proc) (point))))
       (with-current-buffer source-buffer
         (comint-send-region proc start end)
@@ -733,14 +737,15 @@ Afterwards displays the buffer in some window."
           (racket--debug-send-definition (point) end)
         (racket--send-region-to-repl (point) end)))))

-(defun racket-send-last-sexp ()
+(defun racket-send-last-sexp (&optional prefix)
   "Send the previous sexp to the Racket REPL.

 When the previous sexp is a sexp comment the sexp itself is sent,
 without the #; prefix."
-  (interactive)
+  (interactive "P")
   (racket--send-region-to-repl (racket--repl-last-sexp-start)
-                               (point)))
+                               (point)
+                               prefix))

Note: I'm not proposing that you patch this yourself. This is just me sketching out a possible approach. Initial reply while I think about it.

greghendershott commented 2 years ago

I'm less sure about this other request:

Also, when (define something, there is almost no feedback. I think this violated the design principle that action should be associated with feedback because feedback provides reassurance. A lack of feedback creates a feeling of lack of control, which can be unsettling. One cannot know what was defined recently by a simple glimpse.

At the REPL prompt, if you directly enter any expression that returns #<void>, such as (void), then read-eval-print-loop (or probably more precisely the current print handler) prints nothing.

I think most people like this behavior and it's a good default? I guess it would be easy enough to change this to print #<void> instead of nothing, as an option.

As for define: I don't think the name is apparent in the value; it's also just void (or "void-like"). Also I should say names, plural; think of (begin (define x 1) (define y 2) ...).

Instead, I think it would need to fully macro-expend your expression to discover all the primitive define-values, and the identifiers used. This seems possible but a little gross. Anyway that's just my first reaction.

shenlebantongying commented 2 years ago

After reading your response, I realized that the (define doesn't make much sense here too. However, if a user types a defined function name in racket's repl, it does output something -> #<procedure:a-name>.

I did a little survey of define/defun/def in other repl:

It seems only the lisp output the name in repl. And I just realize only in (e)lisp, defun\defvar will returns a symbol. I probably got influenced by this behaviour which i didn't realize explicitly.

My bad, I didn't think this part thoroughly.

greghendershott commented 2 years ago

To a topic branch (not yet merged), I pushed a commit yesterday, where you need to press C-u before racket-send-last-sexp.

Thinking about it more, I figured you'd find that annoying. I just pushed a commit c6991eb (again not yet merged) where instead you can set a new customization variable, racket-repl-copy-expression. When t you'll get the desired behavior. When nil (the default) it's the status quo behavior.

Does this seem reasonable?


There are several "send something to REPL" commands:

But I'm not 100% sure. What do you think?

shenlebantongying commented 2 years ago

I tried the branch. racket-send-last-sexp will throw this message, but racket-send-region does work.

funcall-interactively: Wrong number of arguments: #<subr racket-send-last-sexp>, 1
Debugger entered--Lisp error: (wrong-number-of-arguments #<subr racket-send-last-sexp> 1)
  racket-send-last-sexp(nil)
  funcall-interactively(racket-send-last-sexp nil)
  command-execute(racket-send-last-sexp)

I think the second \n is unnecessary

https://github.com/greghendershott/racket-mode/blob/c6991eb7e28dc99a2562ef15640c724b2a6b1d3d/racket-repl.el#L715 left->with 2nd \n, right-> without 2nd \n

Screen Shot 2022-03-28 at 10 04 13 AM

racket-send-definition/region does look awkward if send a big definition 🤦

Personally, I need racket-send-last-sexp to show what was evaluated mostly. I need to do some demos: After writing some define, I will evaluate some snippets to show how they work and what's going on.

Using a variable to change the behaviour of 3 actions doesn't feel right.

The original approach's mindset be like -> Yes, i need the sexp/region/define printed -> press C-u -> Do it C-x C-e. It feels more reasonable.

Thanks for your sweet help :)

greghendershott commented 2 years ago

I tried the branch. racket-send-last-sexp will throw this message, but racket-send-region does work.

Sorry! I neglected to fully remove the C-u prefix approach.

I'll... not-fix, that in light of...

The original approach's mindset be like -> Yes, i need the sexp/region/define printed -> press C-u -> Do it C-x C-e. It feels more reasonable.

OK. I'll revert to that.

I think the second \n is unnecessary

I think it's important for each resulting value to go in its own line, and not at the end of the ;; -> comment line, for two reasons:

That's why I think it would be better to print ;; => on its own line, followed by zero or more values, one for each value.

With that explanation, does that make sense to you, too?

greghendershott commented 2 years ago

p.s. Even if the expression evaluates to a single value, it might not fit on the one ;; => comment line. For example (for/list ([n 100]) n), with racket-pretty-print set to non nil.

So across the board, I like the standard Racket REPL behavior where it starts printing each of the zero or more values on its own line. And for all the lines to be uniformly not-commented lines.

greghendershott commented 2 years ago

@shenlebantongying I pushed more commits to restore the C-u prefix approach.

Note that it still does print the resulting zero or more values, each starting on its own, non-commented line. This isn't what you originally asked, but I think it's the most reasonable thing to do as explained above.

If that seems OK, please let me know and I'll squash/merge.

shenlebantongying commented 2 years ago

I just tested new commits, and they work perfectly :)

I turned off most syntax, so I didn't notice much difference if "starting on its own, non-commented " or not, but for people having syntax highlights, I think seeing clearer structures is probably better for them. You should keep the decision.

It does fit my demonstration purpose nicely. Thank you for your help!

greghendershott commented 2 years ago

Thank you for the feedback! Squashed and merged to the main branch.