tree-sitter / tree-sitter-ruby

Ruby grammar for tree-sitter
MIT License
182 stars 58 forks source link

Wrap class, module, method, and block bodies in a named node #224

Closed npezza93 closed 2 years ago

npezza93 commented 2 years ago

A majority of other languages I noticed wrap class and function bodies in their own named node that can be easily queried(checked javascript, Lua, java, etc). Without this named node it makes targeting the contents of these structures difficult, cumbersome, and realllyyyy slow.

Checklist:

npezza93 commented 2 years ago

@aibaars Yea I originally had that but it made targeting just the class/module/method, etc. body hard because it was also capturing all other bodies that were a child and such. I reverted back to having just the body named node like before for everything but block but wrapped them in a named field so specific targeting is still preserved. Let me know what you think!

aibaars commented 2 years ago

@npezza93 I don't understand https://github.com/tree-sitter/tree-sitter-ruby/pull/224/commits/9f6775cec28d78bea22e4958593803a5c45b8f4e . Without that commit this PR looks good to me.

maxbrunsfeld commented 2 years ago

Without that commit this PR looks good to me

I agree, we should revert that revert; the fields should all just be body.

One other change I would suggest is to rename body_statement to something without _statement at the end. To me, the name body_statement makes it sound like this is a type of statement, but I don't think it is. What do you think of just calling it body?

aibaars commented 2 years ago

One other change I would suggest is to rename body_statement to something without _statement at the end. To me, the name body_statement makes it sound like this is a type of statement, but I don't think it is. What do you think of just calling it body?

@maxbrunsfeld I had suggested the name body_statement, because it matches the name used in Ruby's parse.y (ie body_stmt). In the grammar of the Ruby interpreter a body_stmt is a "body" that may have rescue and ensure clauses. The tree-sitter rule for this type of body already had this name with an _ before this PR.

maxbrunsfeld commented 2 years ago

Ah yeah, I see Ripper calls it that. Good point. Let's keep it.

olimorris commented 2 years ago

@npezza93 are we ready this awesome pull?

npezza93 commented 2 years ago

I'll finish this up this weekend

npezza93 commented 2 years ago

@aibaars So changed to use a general body name but I don't think it is working correctly. Using a query of (method) body: (body_statement) @function.inner it seems to be selecting the entirety of inside the class instead of the insides of each method. Maybe my query is wrong but this wasn't the case when they had the different name. Any ideas?

aibaars commented 2 years ago

@npezza93 I know very little about the query language. @dcreager could you have a look at what's going wrong here?

maxbrunsfeld commented 2 years ago

Maybe my query is wrong but this wasn't the case when they had the different name. Any ideas?

I think the query would need to have the body field inside of the method node:

(method
  body: (body_statement) @function.inner)

@npezza93 Does that work for you?

npezza93 commented 2 years ago

@maxbrunsfeld Yep, totally works! Thanks!

maxbrunsfeld commented 2 years ago

Ok, so just checking - this PR is done and working as intended, right?

npezza93 commented 2 years ago

@maxbrunsfeld Yep! This is ready to go

maxbrunsfeld commented 2 years ago

Great, thanks for this @npezza93.

olimorris commented 2 years ago

Thanks @npezza93 and everyone involved. This makes writing Ruby queries so much easier. Some of the things we're doing in Neovim with Treesitter are beyond what I ever thought we'd see.

What are the time frames for it to be reflected in the Treesitter Playground?