-
Notifications
You must be signed in to change notification settings - Fork 36
Update to tree-sitter 0.23 #133
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
Conversation
package.json
Outdated
"build": "npx tree-sitter-cli generate --no-bindings", | ||
"prestart": "npx tree-sitter-cli build --wasm -o tree-sitter-r.wasm", | ||
"start": "npx tree-sitter-cli playground", | ||
"test": "npx tree-sitter-cli test", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tree-sitter
invocations were changed to make the scripts a bit more robust. Calling tree-sitter
through npx
like this ensures the correct version from the dev dependencies is used, and makes the scripts independent from the local system state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be changed upstream then? i.e. in https://github.com/tree-sitter/tree-sitter/blob/master/cli/src/generate/templates/package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. This was a misunderstanding on my part. It turns out NPM puts the dev dependency binaries on the path when running scripts, so this was correct. But I see you didn't keep this change anyway 👍
Thanks! I knew this was coming and agree it's a good idea. I'll take a close look at this sometime today or early next week |
|
||
require github.com/tree-sitter/go-tree-sitter v0.23.1 | ||
|
||
require github.com/mattn/go-pointer v0.0.1 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this? It's not in the upstream code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where this came from. Running go mod tidy
added it, and without it, the Go code wouldn't build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a dependency of tree-sitter
:
I don't know why go mod tidy
thinks it should be included here 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I see that many other grammars have it, this one is fine thanks!
Thanks @hendrikvanantwerpen! |
Thanks for the quick turnaround :) Do you think it might be possible to do a release soon as well? It's nicer to have a crate version than a commit dependency. |
🫡 https://crates.io/crates/tree-sitter-r/versions Is this for usage in Code Search? |
Tree-sitter released version 0.23 recently. This contains a breaking, but ultimately very good change to how parser bindings work (tree-sitter/tree-sitter#3069). The TLDR of that change is that parsers do not depend on tree-sitter anymore, but instead on a shard (and supposedly very stable) tree-sitter-language crate. As a result, clients are free to chose their tree-sitter version as the parser ABI is supported. Library and parser versions are less tighly coupled, and it should no longer be necessary to move all the parsers to the next tree-sitter version in lock-step to be able to upgrade the library.
Most of the changes in this PR are from running tree-sitter generate. I'll add comments on other changes I did to explain why I thought they are necessary, or where I'm not sure I did it correctly.