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

Consider all clojure sexps as defuns #32

Merged
merged 2 commits into from
Feb 11, 2024

Conversation

kommen
Copy link
Contributor

@kommen kommen commented Feb 8, 2024

Consider this clojure code, | indicating the point position

(ns my.app)

(defn foo []
  (+ 1 1))

foo|

"another sexp"

(defn bar []
  (+ 2 1))

Without this change, beginning-of-defun would move point before the (defn foo,,,) as the symbol literal foo is not considered as a valid defun.

With this change, all clojure sexps will be considered as defuns, so beginning-of-defun moves point before foo, which I would consider as the expected behavior.

Similar for cider-eval-defun-at-point: At the point position indicated in the example without this change the (defn bar,,,) form is evaluated. With this change, foo is evaluated, which I also would consider the expected behavior.

@@ -917,7 +917,7 @@ See `clojure-ts--font-lock-settings' for usage of MARKDOWN-AVAILABLE."
(setq-local treesit-defun-prefer-top-level t)
(setq-local treesit-defun-tactic 'top-level)
(setq-local treesit-defun-type-regexp
(cons (rx (or "list_lit" "vec_lit" "map_lit"))
(cons (regexp-opt clojure-ts--sexp-nodes)
Copy link
Member

Choose a reason for hiding this comment

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

I'd add some note explaining this, so someone wouldn't fix down the road. :-)

@bbatsov
Copy link
Member

bbatsov commented Feb 11, 2024

I'm fine with the proposal as it's in line with how most Emacs modes work - defuns are essentially top-level forms in them, not real defuns.

I'd suggest adding a couple of unit tests for this (you can copy something from clojure-mode) and a changelog entry, though.

@kommen
Copy link
Contributor Author

kommen commented Feb 11, 2024

@bbatsov thanks for the review! I added the note and the changelog entry. However, for the tests there is nothing set up yet here and I guess adding some bare-bone tests could conflict with @dannyfreeman's plan outlined in #25 (comment)?

@bbatsov
Copy link
Member

bbatsov commented Feb 11, 2024

Well, I see those tests as accretive to the tests that we plan to copy, so I don't think that would create any issues. I'm guessing you can just copy all the tests, as this would help set up the infra here and this would actually speed up the compatibility work.

But if you don't want to tackle this, I or Danny can do it down the road.

@kommen
Copy link
Contributor Author

kommen commented Feb 11, 2024

@bbatsov I'd rather not tackle setting up the tests here and now, but would be happy to help Danny and you down the road.

@bbatsov bbatsov merged commit f11b680 into clojure-emacs:main Feb 11, 2024
1 check passed
@camdez
Copy link

camdez commented Feb 14, 2024

It might be clarifying for the issue title (and more importantly the changelog) if it said "top-level forms" (or even "top-level sexps"). I read the title and thought every (possibly-nested) sexp would be a target for beginning-of-defun, but that appears not to be the case.

(Please disregard entirely if I've misunderstood—I haven't run this.)

Cheers.

@bbatsov
Copy link
Member

bbatsov commented Feb 14, 2024

Yeah, that affects only top-level forms. I'll update the changelog.

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