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

Fix semantic indentation of quoted functions #49

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

rschmukler
Copy link
Contributor

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):

  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

Thanks!

@@ -520,6 +520,10 @@ with the markdown_inline grammar."
"Return non-nil if NODE is a Clojure keyword."
(string-equal "kwd_lit" (treesit-node-type node)))

(defun clojure-ts--quoted-var-node-p (node)
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 the right terminology here is var-quoted-node. That's because you can also have the standard quoting with '.

@@ -520,6 +520,10 @@ with the markdown_inline grammar."
"Return non-nil if NODE is a Clojure keyword."
(string-equal "kwd_lit" (treesit-node-type node)))

(defun clojure-ts--quoted-var-node-p (node)
"Return non-nil if NODE is a Clojure quoted."
Copy link
Member

Choose a reason for hiding this comment

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

Clojure -> var-quoted

@@ -60,6 +60,8 @@
(clojure.core/filter even?
(range 1 10))

(#'filter even?
Copy link
Member

Choose a reason for hiding this comment

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

I the indentation OK if it was just 'filter? Also it might be good to change the example to make more sense in the context of quoting being used.

Copy link
Contributor Author

@rschmukler rschmukler Jul 26, 2024

Choose a reason for hiding this comment

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

I am not sure how 'filter should indent. Specifically, the reason I think a quoted var should indent like this, is because it implements 'IFn (ie. it can be called) while a symbol doesn't. Let me know if you agree with this. On the others, happy to change the name.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm OK with this.

@rschmukler
Copy link
Contributor Author

rschmukler commented Jul 26, 2024

@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
Copy link
Contributor Author

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

@bbatsov
Copy link
Member

bbatsov commented Oct 26, 2024

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

@rschmukler
Copy link
Contributor Author

@bbatsov rebased and ready to go 👍

@bbatsov
Copy link
Member

bbatsov commented Nov 3, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

bbatsov commented Nov 3, 2024

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
Copy link
Contributor Author

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

@bbatsov
Copy link
Member

bbatsov commented Nov 3, 2024

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

Fixes an error where quoted functions would not align correctly with
semantic indentation.

Adds an example test and updates the changelog
@rschmukler
Copy link
Contributor Author

Sorry about that, should be good now

@bbatsov bbatsov merged commit 3ca382c into clojure-emacs:main Nov 4, 2024
3 checks passed
@bbatsov
Copy link
Member

bbatsov commented Nov 4, 2024

Thanks!

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.

2 participants