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

adds missing tree-sitter-comment injection for js/ts #2763

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

farwyler
Copy link
Contributor

this adds the injection for tree-sitter-comment to javascript. i am not sure if there is a reason for this missing. maybe it was supposed to interfere with the injection for "jsdoc" comments?

anyhow, it does seem to work to with both injections on "comment".
Screenshot 2022-06-14 at 06 46 27

@the-mikedavis
Copy link
Member

Yeah they fight with eachother a bit. Actually I can't reproduce the highlights you get:

Master 2763
before after

@farwyler
Copy link
Contributor Author

@the-mikedavis you are right, it is unstable and flip flops around while changing the document. i do consistently get the correct highlighting on first load tough. 😟

it seems to work as expected in neovim. i'll have a closer look.

@archseer
Copy link
Member

You probably want to inject the comment grammar into the jsdoc grammar?

@farwyler
Copy link
Contributor Author

You probably want to inject the comment grammar into the jsdoc grammar?

I am not sure how. The jsdoc injection is already attached to the (comment) node. How would i capture the root node of jsdoc then, to inject the comment parser there?

Doing this for javascript

((comment) @injection.content (#set! injection.language "comment"))
((comment) @injection.content (#set! injection.language "jsdoc"))

... actually works and is how the queries are defined in neovim. The ordering here is actually important.
I don't know if this is a valid approach given the current implementation though.

If it is, there seems to be a small issue here:

pub fn update(

When updating tree-sitter, the language layers for the injections are added in order as they are discovered in the document. In our case of comment fighting with jsdoc at the same node, it only produces the correct highlighting if the language layer for comment comes before jsdoc.

So is it technically and implemenation-wise valid to have two parsers capture the same node? And if so, shouldn't the ordering of layers for the injections be stable as it is defined in the queries?

Feedback much appreciated on this. I would really like to get a better grasp of the implementation here.

@the-mikedavis
Copy link
Member

The root node for jsdoc is document but actually we can be more specific and inject comment into description which covers a bunch of text:

; runtime/queries/jsdoc/injections.scm
((description) @injection.content
 (#set! injection.language "comment"))

@farwyler
Copy link
Contributor Author

The root node for jsdoc is document but actually we can be more specific and inject comment into description which covers a bunch of text:

; runtime/queries/jsdoc/injections.scm
((description) @injection.content
 (#set! injection.language "comment"))

jsdoc.document only captures multiline comments, so injecting comment via jsdoc will remove the comment highlighting from all inline comments.

sadly the javascript parser does not differentiate between single and multiline comments, as far as i can tell.

@the-mikedavis
Copy link
Member

You can tell them apart using #match? in queries:

; runtime/queries/javascript/injections.scm

; block comment:
((comment) @injection.content
 (#set! injection.language "jsdoc")
 (#match? @injection.content "^/\\*.*\\*/$"))

; line comment:
((comment) @injection.content
 (#set! injection.language "comment")
 (#match? @injection.content "^//"))

@farwyler farwyler force-pushed the ts-js-todo-comments branch from 280226e to f545c11 Compare June 20, 2022 15:49
@farwyler
Copy link
Contributor Author

This works now in all kind of comment scenarios.

@the-mikedavis thanks for the support! fyi, i am using (document) instead of (description) because description seems to be specific to text following jsdoc @tags and not the whole comment.

/**
 * some description
 * FIXME: this is not (description)
 * @param {number} this counts as (description)
 * @returns
 */

@the-mikedavis
Copy link
Member

Ah yeah good catch, let's use document 👍

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Gave it a spin and this looks good. Thanks!

@the-mikedavis the-mikedavis merged commit cad4e03 into helix-editor:master Jun 20, 2022
@farwyler farwyler deleted the ts-js-todo-comments branch June 20, 2022 15:56
lazytanuki pushed a commit to lazytanuki/helix that referenced this pull request Jun 21, 2022
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