Skip to content

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

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.

Sign up for GitHub

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

Merged
theHamsta merged 1 commit into from
Jan 13, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions queries/lua/textobjects.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@
(#make-range! "parameter.outer" @parameter.inner @_end))

(table_constructor
. (_) @parameter.inner
. ","? @_end
(field) @parameter.inner
","? @_end
Comment on lines +79 to +80
Copy link
Member

@lucario387 lucario387 Jan 14, 2024

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

@lucario387 lucario387 Jan 14, 2024

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

@ribru17 ribru17 Jan 14, 2024

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

@qstrahl qstrahl Jan 14, 2024

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

(#make-range! "parameter.outer" @parameter.inner @_end))

(arguments
Expand All @@ -90,11 +90,6 @@
. (_) @parameter.inner
(#make-range! "parameter.outer" @_start @parameter.inner))

(table_constructor
"," @_start
. (_) @parameter.inner
(#make-range! "parameter.outer" @_start @parameter.inner))

; number
(number) @number.inner

Expand Down