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

fix(lua): ignore comments for table inner param #549

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

ribru17
Copy link
Member

@ribru17 ribru17 commented Jan 10, 2024

This PR makes it so that (comment) nodes are ignored when recognizing @parameter.(inner|outer) textobjects in Lua tables. We now only capture (field) nodes. This helps with swapping and deletion, when doing both actions while having comments inbetween fields, currently the behavior is quite buggy because the (_) query will capture comments, and improperly since they do not have the associated "," node. This PR also removes a query to simplify and improve captures, and it is not necessary because tables (unlike the similar parameter and argument queries) can have trailing commas, so we don't need to create any finnicky captures. To test, notice the difference when swapping @parameter.inner with something like the following:

local swapme = {
  -- comment
  'fieldhere',
  'anotherfield',
  -- another comment
  'a final field',
}

@qstrahl
Copy link

qstrahl commented Jan 10, 2024

While table fields can have trailing commas, they don't have to - simplifying to one query in this case creates situations where operations like deleting a @parameter.outer can change your code style from eschewing trailing commas to having them.

I actually created a draft PR to address this just recently (#548). I expect table constructors are not the only thing in lua where comments create issues, and I expect lua is not the only language with these issues either. I'd greatly appreciate any additional support you can spare on this. 🙏

@ribru17
Copy link
Member Author

ribru17 commented Jan 10, 2024

Ah, you're right. Thank you, that is a good point. I'm not quite sure either, but I do know that parameter and argument lists also suffer from this problem. Sadly I think argument lists are not as easily solved: parameter lists have either an identifier or a comment node, tables have either field or comment, but arguments can have any kind of expression node (or comment). So they are harder to match... But I do like your approach of including the comments with the match! I think this makes the most sense.

@theHamsta theHamsta requested a review from kiyoon January 10, 2024 20:55
@theHamsta
Copy link
Member

theHamsta commented Jan 10, 2024

Thank you for your contribution! Looks good to me!

@theHamsta theHamsta merged commit a97a6ea into nvim-treesitter:master Jan 13, 2024
5 of 6 checks passed
Comment on lines +79 to +80
(field) @parameter.inner
","? @_end
Copy link
Member

Choose a reason for hiding this comment

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

Any pairs of field and , will create a mapping. I don't think that's intended?

Copy link
Member

Choose a reason for hiding this comment

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

If anything, the original version should be updated to

(table_constructor
  . (comment)*
  . (field)
  . ",")

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you are saying. I was wondering about this, but it seems that even the general capture works properly (searching the closest comma-field pairing). I went with this query group because it simplified things, I think for complete correctness the other query would have to be:

(table_constructor
  . (comment)*
  . (field)
  . (comment)* ; potential inline comments
  . ",")

This would rarely (if ever) happen, but it still could, and this query looks a lot weirder than the other simpler one that still works despite capturing maybe many more pairings. Unless you are saying that the captures themselves are an issue (maybe speed-wise?)

Copy link

Choose a reason for hiding this comment

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

Unfortunately that query starts breaking once you try to put captures in, and you end up having to do something a little less elegant to cover these cases, like I did in the aforementioned draft PR

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.

4 participants