clojure-emacs / clojure-ts-mode

The next generation Clojure major mode for Emacs, powered by TreeSitter
GNU General Public License v3.0
140 stars 13 forks source link

Fix semantic indentation of quoted functions #49

Closed rschmukler closed 2 weeks ago

rschmukler commented 4 months ago

Fixes an error where quoted functions would not align correctly with semantic indentation. Adds an example to the test sample.


Before submitting a PR mark the checkboxes for the items you've done (if you think a checkbox does not apply, then leave it unchecked):

Thanks!

rschmukler commented 4 months ago

@bbatsov Upon thinking about this further, I think that the proper name should actually be var-node-p or variable-node-p. The problem is that there is already a variable-node-p which returns whether a node is a def or defonce. I think we should do the following:

  1. Rename the existing variable-node-p to variable-definition-node-p
  2. Rename quoted-var-node-p to var-node-p

I think calling it anything else will be a bit confusing since it is technically a variable... ie:

(type #'filter)
;; => clojure.lang.Variable

I have updated the PR with these changes, but I am also happy to change it to whatever you want. Let me know!

rschmukler commented 3 months ago

@bbatsov just pinging you on this. No rush, just a reminder :)

bbatsov commented 4 weeks ago

@rschmukler Sorry about me dropping the ball on this earlier. You'll need to rebase on master and we can wrap it up.

rschmukler commented 3 weeks ago

@bbatsov rebased and ready to go 👍

bbatsov commented 3 weeks ago

Can you please address the small lint error that's causing the lint check to fail?

Also - now that we have some unit tests (see #57), you might want to add one covering your change.

rschmukler commented 2 weeks ago

@bbatsov the PR is updated with listing passing. I tried to make the tests run and added (what I thought was an appropriate case to clojure-mode-indentation-test.el -however it looks like some of the tests in there aren't running? Specifically I was adding it to the describe "should handle function spec" section. Something like:

(when-indenting-it "should handle var based funcalls with style 'align-arguments"
    'align-arguments

    "
(#'some-function
  10
  1
  2)"

    "
(#'some-function 10
                 1
                 2)")

However when I run make test I don't see any of the function related indentation tests running. Let me know what I am missing here.

bbatsov commented 2 weeks ago

Those are the actual tests that we currently run https://github.com/clojure-emacs/clojure-ts-mode/blob/main/test/clojure-ts-mode-indentation-test.el

The ones in the other folder need to be ported to the new test suite.

rschmukler commented 2 weeks ago

Got it! Thanks for the explanation and sorry for missing that. Updated the test suite to include a unit test for this.

bbatsov commented 2 weeks ago

Hmm, seems that on Emacs snapshot there's a load error for the test file.

rschmukler commented 2 weeks ago

Sorry about that, should be good now

bbatsov commented 2 weeks ago

Thanks!