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

Support toplevel defs with metadata #43

Closed
wants to merge 23 commits into from

Conversation

kommen
Copy link
Contributor

@kommen kommen commented Mar 31, 2024

Fixes #42

Pushing this as WIP and to collect feedback from others if this is the right approach.

  • don't indent a toplevel def with metadata
  • font lock toplevel def with metadata
  • font lock toplevel def symbol name
  • make toplevel def with metadata findable via imenu
  • don't indent a toplevel vector, set or map with metadata
  • font lock docstrings of def/defn forms with metadata

@bbatsov
Copy link
Member

bbatsov commented Jun 4, 2024

I guess that's still waiting for feedback, right?

@kommen
Copy link
Contributor Author

kommen commented Jun 4, 2024

@bbatsov yes, feedback very welcome! however, I'm using this PR in my prod emacs setup for a few months now and it seems to work as intended. So I would be somewhat confident to merge it for now as is and continue with the remaining todos in a follow up, if no one else has objections.

@@ -349,7 +349,7 @@ with the markdown_inline grammar."

:feature 'builtin
:language 'clojure
`(((list_lit :anchor (sym_lit (sym_name) @font-lock-keyword-face))
`(((list_lit meta: _ :? :anchor (sym_lit (sym_name) @font-lock-keyword-face))
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets you some of the way to syntax highlighting with these types of forms. A similar fix I believe is also needed for just about anything that follows the pattern of matching

(list_lit :anchor (sym_lit (sym_name @capture)  ...) ...)

Because any list literal can have metadata attached to the beginning. A lot of these queries want to capture the first symbol in the list to match special function calls, so they had to be :anchored to the front of the list. They all break down when metadata is present because the syntax tree lists the metadata as the first literal sub element of the list node.

This also applies to the docstring queries in clojure-ts--docstring-query.

(defn a
 "hello" ;; <- highlighted with font-lock-doc-face
  [] "world")

and

^{:cat :dog}
(defn b
 "hello" ;; <- not highlighted
  [] "world")

The "hello" string in both should be highlighted with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't get this to work for now. Shall I revert my change related to this and get the PR merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure. I need to set aside some time to get reacquainted with this problem. I'll try to do that within the next week and have a better answer for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dannyfreeman thank you! meanwhile, I had a chance to dive deeper and I think I actually figured it out (mostly?). there are a bunch of similar queries where the the optional metadata node could be added, but I didn't have time to come up with actual clojure code examples testing them so a didn't want to touch them – maybe unnecessarily.

(let* ((grandparent (treesit-node-parent parent)))
(and (string-equal "source" (treesit-node-type grandparent))
(clojure-ts--list-node-p parent)
(treesit-node-child-by-field-name parent "meta"))))
Copy link
Contributor

Choose a reason for hiding this comment

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

The indent rules are some of the hardest to get right. The current indent rule that picks up the def is

Matched rule: ((parent-is "list_lit") parent 1

Which I believe is happening because the when the cursor is between the metadata and the def form ^{:a 1}|(def ...) the parent is technically the list.
I think what we want is a rule that comes before (parent-is "list_lit"), which matches when the parent is a list and the first sibling is meatadata, to indent 0. That might need a special function.

Trying to match the source string at the top should not be done though. That will not apply the rule to anything nested

(comment
  ^{:a 1}|(def a 2)
  )

Copy link
Contributor Author

@kommen kommen Oct 15, 2024

Choose a reason for hiding this comment

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

@dannyfreeman thanks. I addressed this and added some test cases and checked that it matches the behavior of clojure-mode on them.

@kommen
Copy link
Contributor Author

kommen commented Oct 19, 2024

Functionality wise this PR now seems complete to me: I think some of the code needs a bit of clarification and better naming.

I will use my recent changes for a while myself in my daily workflow before I will ask for merging this PR. Meanwhile if folks want to give it a try or discuss the changes with me (here or also on the Clojurians slack), I would appreciate this very much.

I'd be also up to split out some changes in their own PR if others are more comfortable to review smaller chunks (ie. font lock fixes, imenu fixes, indentation fixes).

@bbatsov
Copy link
Member

bbatsov commented Oct 19, 2024

I agree it'd better if we split this into several smaller PRs.

@kommen
Copy link
Contributor Author

kommen commented Oct 19, 2024

@bbatsov agreed. I will start splitting it into separate PRs once I've had a chance to use it a while all together.

@kommen
Copy link
Contributor Author

kommen commented Oct 23, 2024

As discussed, splitted out the first set of changes to fix the imenu integration here: #54

@kommen
Copy link
Contributor Author

kommen commented Oct 25, 2024

And the font locking changes here: #55

@bbatsov
Copy link
Member

bbatsov commented Oct 27, 2024

Now that #54 and #55 have been merged we can close this PR.

@bbatsov bbatsov closed this Oct 27, 2024
@kommen
Copy link
Contributor Author

kommen commented Oct 27, 2024

There are still the indentation fixes from this PR which I still will separate out and as with #55 try to port some relevant test utilities over from clojure-mode

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.

Toplevel defs with metadata are not handled properly
3 participants