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

refactor: decouple tree_sitter grammar #37

Merged
merged 8 commits into from
Mar 25, 2024
Merged

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Mar 16, 2024

The PR detangles how we currently interact with tree_sitter.

While the latest state of the PR gives us one tree_sitter_v module for the api bindings and the grammar, those distinct features are still decoupled and self standing. This should make things more structured and straightforward to use and easier to test.

Quicklinks if comparing the project trees in the github UI helps:

@spytheman
Copy link
Member

Please rebase over latest main.

@spytheman
Copy link
Member

Just tree_sitter is a bad module name imho, since it is easy to confuse it with the tree sitter project itself. Can we use something more distinctive?

@spytheman
Copy link
Member

Even a code name like vitter or graffiti will be better imho.

@spytheman
Copy link
Member

Or tree_sitter_wrapper ?

@ttytm
Copy link
Member Author

ttytm commented Mar 16, 2024

It's a binding to tree_sitter and enables using the official tree-sitter project. So it will never get closer for a V module. That's why I thought it's not confusing.

@ttytm
Copy link
Member Author

ttytm commented Mar 16, 2024

So personally I would keep it. From the suggestions I like tree_sitter_wrapper best so far.

@ttytm
Copy link
Member Author

ttytm commented Mar 16, 2024

Gonna rebase this evening. On run at the airport now.

@spytheman
Copy link
Member

Have a nice trip.

@ttytm ttytm force-pushed the refactor/decouple branch from 7c5fcdb to 48ad7f1 Compare March 16, 2024 20:05
@ttytm
Copy link
Member Author

ttytm commented Mar 16, 2024

Thanks!

It wasn't clear from the comments reactions to me, the bindings name remained with the rebase. You have the veto on this.

@JalonSolov
Copy link

I know it's a common way to reduce typing, but do we really want to have import tree_sitter as ts?

First, ts makes me think typescript not tree_sitter.

Second, V likes to be more explicit.

I think it helps readability to keep the longer name, even if it makes the lines a bit longer.

@ttytm ttytm marked this pull request as draft March 17, 2024 06:44
@ttytm
Copy link
Member Author

ttytm commented Mar 17, 2024

@spytheman what about tree_sitter_api?

then as api @JalonSolov ?

@JalonSolov
Copy link

I'd be ok with import tree_sitter_wrapper as wrapper, I think. At least it's a bit more obvious what's going on.

Not a fan of import tree_sitter { NodeType, Tree } to get around typing the module name, either, as you can't tell from the code where NodeType is defined unless you scan the import lines. Otherwise, it looks like it's declared in the current module. :-\

@ttytm
Copy link
Member Author

ttytm commented Mar 17, 2024

First, ts makes me think typescript not tree_sitter.

Yes but if anyone works in the project where is no typescript usage and but tree_sitter the brain should be able to make the connection. At least for my brain there's no issue with it. I tried to adapt and making it better but imo the last commit is a worsening. So we disagree here.

Otherwise, it looks like it's declared in the current module. :-\

So this is a general dislike against Vs feature for selective imports

@JalonSolov
Copy link

I disagree about selective imports because it makes the code less understandable. Less explicit.

All it is, is a way to "hide" where something came from, just to have shorter things to type.

@ttytm ttytm marked this pull request as ready for review March 17, 2024 18:54
@spytheman
Copy link
Member

@spytheman what about tree_sitter_api?

then as api @JalonSolov ?

I am fine with that, and any other name, as long as the folder is not named tree_sitter.

It may seem pedantic, and petty, and I am sorry, but I really do not want to have any possibility of confusion with the tree sitter project itself.

@spytheman
Copy link
Member

Btw, TypeScript and thus ts, is used for the vscode extension, which is in ./editors/code, although it is unlikely that it will cause confusion, since the name conflict is in another level.

@ttytm ttytm force-pushed the refactor/decouple branch from 0171615 to 89a7ca1 Compare March 18, 2024 17:47
@ttytm ttytm marked this pull request as draft March 19, 2024 22:22
@ttytm ttytm added this to the 0.0.5 milestone Mar 20, 2024
@ttytm ttytm force-pushed the refactor/decouple branch 3 times, most recently from d01a9af to 5b48885 Compare March 23, 2024 21:45
@ttytm
Copy link
Member Author

ttytm commented Mar 23, 2024

Brought it into a project structure that hopefully makes all of us happy. It hopefully fulfills everything in terms of decoupling, structuring and naming.

The binding and grammar now both can reside in tree_sitter_v. The binding is accessible under tree_sitter_v.bindings.

@ttytm ttytm marked this pull request as ready for review March 23, 2024 21:55
@ttytm ttytm force-pushed the refactor/decouple branch from 5b48885 to 3ae7010 Compare March 25, 2024 13:15
@ttytm ttytm merged commit bf89fe4 into vlang:main Mar 25, 2024
13 checks passed
@ttytm ttytm deleted the refactor/decouple branch March 25, 2024 21:43
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