vlang / v-analyzer

The @vlang language server, for all your editing needs like go-to-definition, code completion, type hints, and more.
MIT License
92 stars 10 forks source link

refactor: decouple tree_sitter grammar #37

Closed ttytm closed 5 months ago

ttytm commented 5 months ago

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 commented 5 months ago

Please rebase over latest main.

spytheman commented 5 months ago

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 commented 5 months ago

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

spytheman commented 5 months ago

Or tree_sitter_wrapper ?

ttytm commented 5 months ago

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 commented 5 months ago

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

ttytm commented 5 months ago

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

spytheman commented 5 months ago

Have a nice trip.

ttytm commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@spytheman what about tree_sitter_api?

then as api @JalonSolov ?

JalonSolov commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

spytheman commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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.