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

Tree-sitter rolling fixes, 1.117 edition #974

Merged

Conversation

savetheclocktower
Copy link
Sponsor Contributor

Getting a head start on this one.

This PR starts by adding a modern Tree-sitter grammar for SCSS. For ages, there wasn't a grammar out there that had good enough support for SCSS's features, but I forked this one a while back and very gradually fixed the things that needed to be fixed. I think it's good enough now to bundle and ship and see if there are any edge cases that need to be addressed.

A few days ago, one of the main Tree-sitter contributors made this parser, and I could see us migrating to it in the future (I don't love being a parser maintainer myself)… but it's not there yet. If it ever passes the tests that exist in my fork, then it'd be worth switching to.

@savetheclocktower savetheclocktower marked this pull request as draft April 14, 2024 22:03
@DeeDeeG
Copy link
Member

DeeDeeG commented Apr 21, 2024

Is it worth reaching out to the "one of the main Tree-sitter contributors" and be so bold as to ask if they'd like to adopt your parser, or combine efforts until there's one parser that meets both of your respective sets of expectations? (I dunno if this suggestion is a little "out there", but it came across my mind.)

Anyway, good to see language support expanding for modern tree-sitter.

@savetheclocktower
Copy link
Sponsor Contributor Author

Is it worth reaching out to the "one of the main Tree-sitter contributors" and be so bold as to ask if they'd like to adopt your parser, or combine efforts until there's one parser that meets both of your respective sets of expectations? (I dunno if this suggestion is a little "out there", but it came across my mind.)

No, it's a fine suggestion. He knows about my parser, but he didn't see it until after he'd made his own. I told him about my test corpus and I expect him to adopt it at some point, but I also trust his solutions to some problems more than I trust my own (which are sometimes hacky) and his parser “extends” the Tree-sitter CSS parser more formally than mine does, so I think his is a better foundation to start from.

For now, though, it's not ready.

@savetheclocktower
Copy link
Sponsor Contributor Author

Taking this out of draft so we have time to get it reviewed before 1.117.

@savetheclocktower savetheclocktower marked this pull request as ready for review May 12, 2024 00:22
Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Not a huge one this time, especially if looking at it commit-by-commit.

I am largely going by faith, since I don't know a ton of the details of the scopes/grammars myself. But this looks okay! FWIW that I say that!

Thanks as always for maintaining the grammars!


[language-gfm] Make each block-level HTML tag its own injection…

…which, surprising as it sounds, has a huge performance benefit.

Surprising indeed!

[language-sass] Add SCSS Tree-sitter grammar

Sounds good to me!

[language-ruby] Update to latest Tree-sitter Ruby parser

Also sounds good!

@@ -902,6 +903,20 @@
"." @keyword.operator.accessor._LANG_
"?." @keyword.operator.accessor.optional-chaining._LANG_

; Optional chaining is illegal…
Copy link
Member

Choose a reason for hiding this comment

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

Unsure of the factuality of this comment, perhaps

Suggested change
; Optional chaining is illegal…
; Optional chaining should be illegal…

/s (as in, just kidding!)


Ah, I understand now, from reading the corresponding comment fragments below! So, there are specific contexts in which optional chaining is illegal. Got it. Definitely never mind this, then...


[
"&&="
Copy link
Member

Choose a reason for hiding this comment

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

"+="
"-="
"*="
"**="
Copy link
Member

Choose a reason for hiding this comment

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

@savetheclocktower savetheclocktower merged commit 5024f05 into pulsar-edit:master May 14, 2024
104 checks passed
@savetheclocktower savetheclocktower deleted the tree-sitter-may branch May 14, 2024 05:53
@savetheclocktower savetheclocktower restored the tree-sitter-may branch May 14, 2024 05:55
@savetheclocktower savetheclocktower deleted the tree-sitter-may branch May 14, 2024 05:55
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.

None yet

2 participants