cider icon indicating copy to clipboard operation
cider copied to clipboard

Idea: make more commands work with point at beginning of sexp

Open yuhan0 opened this issue 5 years ago • 7 comments

Is your feature request related to a problem? Please describe.

I usually want to call commands like cider-inspect, and cider-macroexpand with my cursor point at the beginning or middle of a form, instead of at the end like Cider expects.

Describe the solution you'd like

It's mainly a personal preference and habit, so I had cider-last-sexp edited to the following, which moves the point to the appropriate position. Because lots of commands use this under the hood, it automatically makes them able to work with the point at beginning of a paren or symbol.

(defun cider-last-sexp (&optional bounds)
  "Return the sexp preceding the point.
If BOUNDS is non-nil, return a list of its starting and ending position
instead."
  (apply (if bounds #'list #'buffer-substring-no-properties)
         (save-excursion
           (unless (eq (point) (cdr (bounds-of-thing-at-point 'sexp)))  ;; <== added 
             (forward-sexp 1))                                          ;; <== added 
           (clojure-backward-logical-sexp 1)
           (list (point)
                 (progn (clojure-forward-logical-sexp 1)
                        (skip-chars-forward "[:blank:]")
                        (when (looking-at-p "\n") (forward-char 1))
                        (point))))))

Describe alternatives you've considered nil

Additional context This fix means that the command names and docstrings are no longer accurate - should be something like "sexp at point" instead of "preceding sexp"

yuhan0 avatar Apr 05 '20 09:04 yuhan0

A lot of time has passed, so I don't quite recall my original reasoning, but I do think that I borrowed the notion of last-sexp from emacs-lisp-mode and SLIME. The terminology side is pretty tricky, as "sexp-at-point" might actually be surrounding instead of the preceding sexp (at least for some people).

Whatever we decide to do it's very important to be consistent and not to affect the current workflow of CIDER's users. One option would be introduce variants of some commands and configuration what flavour to use. Another would be to make a more generic "cider-current-sexp/sexp-at-point" that works on parens and right after them and just alias cider-last-sexp to it. I definitely agree this can be improved, we just have to find the best approach.

bbatsov avatar Apr 05 '20 09:04 bbatsov

Yeah, I realise that the change above is pretty much a personal hack, but I saw on the Slack channel that someone else had the same preference, so decided to post this in case others find it useful.

I'm also very hesitant to touch something like this in the main library in case it breaks someone else's config (like what happened with clojure-mode syntax table)

But it's worth noting the commands still work as before when point is at end of a sexp, the only difference is in a situation like:

(foo 1)   |(bar 2) 
          ^
          cursor

;; also with symbols
foo |bar

The current behaviour would eval /inspect/macroexpand the foo form, which is arguably less intuitive - one would normally have the point right after the closing paren when navigating.

yuhan0 avatar Apr 05 '20 10:04 yuhan0

Yeah, I realise that the change above is pretty much a personal hack, but I saw on the Slack channel that someone else had the same preference, so decided to post this in case others find it useful.

There's always room for improvement. I like your suggestion overall, I'm just wondering how to best fit it in.

I think it's going to be safest if we change the name of the cider-last-sexp command to something more neural and added configuration controlling its exact behaviour. Alternatively we can just keep the old command and add a new one alongside it.

We'll also have to check how many variations of the cider-last-sexp are there in the code already, as I think there's something like cider-sexp-at-point.

bbatsov avatar Apr 08 '20 08:04 bbatsov

this would be awesome :) in my opinion we should Alternatively we can just keep the old command and add a new one alongside it. it becomes a growing change instead of a breaking change and everybody gets happy!

pablo-develop avatar Apr 09 '20 12:04 pablo-develop

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

stale[bot] avatar Jul 09 '20 03:07 stale[bot]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

stale[bot] avatar Aug 08 '20 06:08 stale[bot]

@yuhan0 FWIW, I'm still interested in improving this. I guess you can just submit your proposal and I'll think about how to polish things down the road.

bbatsov avatar Aug 08 '20 06:08 bbatsov