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(elixir): separate function from parameter matching #383

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skbolton
Copy link
Contributor

@skbolton skbolton commented Feb 8, 2023

Previous queries were mixing the parameter matches and the function
matches into the same query. This would make it so that functions that
didn't have parameters wouldn't match @function.inner or @function.outer
queries.

Separating them makes it easier to focus on the specifics of fulfilling
each query. This should resolve issue #249.

Previous queries were mixing the parameter matches and the function
matches into the same query. This would make it so that functions that
didn't have parameters wouldn't match @function.inner or @function.outer
queries.

Separating them makes it easier to focus on the specifics of fulfilling
each query. This should resolve issue nvim-treesitter#249.
@skbolton skbolton force-pushed the fix-elixir-function-bodies branch from b2f62b2 to 5822de7 Compare February 8, 2023 04:03
@skbolton skbolton marked this pull request as ready for review February 8, 2023 04:05
@skbolton
Copy link
Contributor Author

skbolton commented Feb 8, 2023

Sorry @kiyoon for the barage of PR's I was trying to break up my work into smaller pieces. I wasn't sure all who is involved in this project and how many eyes would see it.

@kiyoon
Copy link
Collaborator

kiyoon commented Feb 8, 2023

When I test it, it seems to be not working with functions with parameters in #249 examples?

@kiyoon
Copy link
Collaborator

kiyoon commented Feb 8, 2023

Thanks a lot for your work, and no worries, it makes it easier to focus on one problem at a time.

Copy link

@yihuikhuu yihuikhuu left a comment

Choose a reason for hiding this comment

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

Changing the following appears to address the remaining issues mentioned in #249.

The binary_operator case captures the guard scenario.

The identifier case addresses another similar issue when the braces are omitted for parameterless functions:

def foo, do: 12
def foo do
  12
end

(do_block "do" . (_) @_do (_) @_end . "end")
(do_block "do" . ((_) @_do) @_end . "end")
]
(arguments (call))

Choose a reason for hiding this comment

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

Suggested change
(arguments (call))
(arguments ([
(call)
(identifier)
(binary_operator)
]))

"defnp"
"defp"
))
(arguments (call))

Choose a reason for hiding this comment

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

Suggested change
(arguments (call))
(arguments ([
(call)
(identifier)
(binary_operator)
]))

(arguments ((_) @parameter.inner) @_delimiter .)
] (#make-range! "parameter.outer" @parameter.inner @_delimiter))
(arguments
(call)

Choose a reason for hiding this comment

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

Suggested change
(call)
([
(call)
(identifier)
(binary_operator)
])

@skbolton
Copy link
Contributor Author

Thanks for the reminder on this PR @yihuikhuu. I'll take another stab at this incorporating your feedback this week.

@calops
Copy link

calops commented Apr 18, 2024

I should have checked existing PRs before opening #605 😅

Mine seems to be a lot simpler, but maybe it's too generic? I've tested it a bit and it seems to match all call parameters in every situation (and it helps simplify other queries as well) without breaking other stuff, but maybe I'm missing something.

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