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

feat: Add feature to distinguish attribute directives & modifiers #9

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AlbertMarashi
Copy link

@AlbertMarashi AlbertMarashi commented Sep 9, 2024

I've added support for enabling the independent styling of directives & modifiers in order to migrate Zed's svelte support to this tree-sitter-svelte instead of https://github.com/Himujjal/tree-sitter-svelte

This will allow us to close some long-standing svelte issues within zed-industries/zed#17529

Supports parsing:

  • class:foo={...}
    • (attribute_directive) (attribute_identifier)
  • on:click|stopPropagation|preventDefault={...}
    • (attribute_directive) (attribute_identifier) (modifier) (modifier)
  • style:--bar={...}
    • (attribute_directive) (attribute_identifier)

All are still wrapped within (attribute_name) node. I'm only familiar with Zed's syntax highlighting conventions, so I left highlights.scm and etc as they currently are

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Sep 9, 2024

Also, I was very confused about the addition of all the new files when I merged from main, and how the package.json no longer contains all of the tree-sitter commands?

Was this supposed to be committed?

@clason
Copy link

clason commented Sep 9, 2024

tree-sitter generate --no-bindings if you only wish to update the parser without touching the bindings (which indeed got more numerous with tree-sitter 0.23.0).

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Sep 9, 2024

I did format all the tests with https://github.com/Sarrus1/tree-sitter-tests-formatter cli

Hopefully it isn't a problem 🙏🏼

edit: I added some contribution docs to the README.MD so others can know how to run the tests and regenerate the parser

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Sep 11, 2024

@amaanq @savetheclocktower I'd love to get this merged so we can get it integrated into Zed's downstream extension!

grammar.js Outdated Show resolved Hide resolved
grammar.js Outdated Show resolved Hide resolved
Copy link
Contributor

@savetheclocktower savetheclocktower left a comment

Choose a reason for hiding this comment

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

I don't have the commit bit on this repo, but @amaanq has enough responsibilities already, so I'll try to reduce the friction on this one. :-)

This is a soft rejection that would change to an approval pretty easily once the feedback is addressed.

@amaanq, there are new nodes introduced — attribute_identifier, attribute_directive, and attribute_modifier (if my feedback is agreeable). For most ordinary attributes, the only effect will be to make attribute_name have a single attribute_identifier child. That doesn't bother me, but please weigh in if you have other suggestions for how this could be handled.

@clason
Copy link

clason commented Sep 12, 2024

The main question is whether the grammar change breaks existing queries. (The repo contains them for a reason.) Adding new nodes is fine, but moving nodes usually isn't. ("grammar semver")

@AlbertMarashi
Copy link
Author

The main question is whether the grammar change breaks existing queries. (The repo contains them for a reason.) Adding new nodes is fine, but moving nodes usually isn't. ("grammar semver")

No, it shouldn't.

Previous queries targeting

(attribute_name) @attribute

For example should continue working

Also alongside the more specific queries, eg:

(attribute_name
    (attribute_identifier) @attribute
    (attribute_modifier) @function
)

@AlbertMarashi
Copy link
Author

@amaanq could we get a look at this? We're currently using this commit reference upstream in zed, so would be good to get it merged!

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