-
Notifications
You must be signed in to change notification settings - Fork 202
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
Make parameter.outer delete the preceeding comma in python, dart, rust, c/c++, and some other changes #87
Conversation
…guish last argument and first argument
This reverts commit 909782a. Oops
…the argument for the parameter in the middle.
I just realize such implementation haven't accounted for if there is only one argument.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! For me this absolutely makes sense. @stsewd can you have a look onto this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mostly looks good, thanks for your work. Noticed that in C for example one parameter functions don't match anymore, better to write the queries for inner parameter in another query from the outer ones, so we avoid this kind of bugs.
@@ -61,9 +61,15 @@ | |||
(_) @statement.outer) | |||
|
|||
((parameter_list | |||
(parameter_declaration) @parameter.inner . ","? @_end) | |||
"," @_start . (parameter_declaration) @parameter.inner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can write the @parameter.inner
in another query, so we don't have duplicates and they also match functions with one parameter. Same for the other queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possible, although I found another way to fix single parameter lists. If we do extract parameter.inner to a separate query then that would essentially be three queries for every kind of parameter. having 2 already feels unnecessary but I dont know how to do it more elegantly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having three queries is more explicit, and we don't have to worry about always trying to capture the inner parameter, and that's one less query for treesitter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always have to capture the @parameter.inner symbol as part of constructing the @parameter.outer range anwyay. (unless there is a way to do this more directly that I am not familiar with?)
It would just be s/@parameter.inner/@_par
and adding a third query for the parameter inner.
If there are performance implications it would be interesting to check, is there anyway to benchmark this?
Single parameter lists are fixed anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would just be s/@parameter.inner/@_par and adding a third query for the parameter inner.
Yeah, that's more explicit and would avoid bugs when changing those queries, like the one that was included.
If there are performance implications it would be interesting to check, is there anyway to benchmark this?
Don't know how to benchmark, but I think these would just generate two matches for parameter.inner
instead of just one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see how it would be more explicit but I don't see how it would avoid bugs? if you wanted to change which nodes are parameters you would have to change 3 places most likely, (right now you have to change two places which is not great either)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you wanted to change which nodes are parameters you would have to change 3 places most likely
but you won't need to worry about changing or breaking parameter.inner
, that one would always work, the other queries will only need to change @_end
or @_start
for parameter.outer
I have pushed a few fixes, please review again, thanks |
Some things left for this issue are:
|
The dart queries have a problem that I am not familiar enough with dart syntax (or the tree-sitter-dart parser) to really test if any fix works. Should I leave it out? |
I am working on javascript and typescript parameter objects, however thats more work as it doesnt even have parameter.outer currently. Should I add those to this PR or let this be merged? |
That can be done later in another PR |
Is there anything else left to be done before merging this PR? |
No, let's merge! 🎉 |
Fixes #85
This will actually delete the preceeding comma for all arguments expect the first parameter. This leaves the argument list better formatted in most cases (by not leaving an extra space as it used too). The first parameter will still leave a space, which we don't know how to fix currently
This PR also makes parameter.outer delete entries from tuples, list and dictionaries in python.
Also, It makes parameter.* cover template parameter/argument lists and lambda capture lists in c++.
(related to #86)
We may open some more pull requests for correcting parameter handling in other languages as we get time