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

make @function.outer and @class.outer for python always use the decorated version #648

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

feakuru
Copy link
Contributor

@feakuru feakuru commented Jul 17, 2024

See #638 for context.

This is a minor adjustment to the Python queries that I find to produce a more useful behavior: I don't ever need to select a function without its decorators, because when I select a whole function, it's to move it, delete it or copy it in whole, and in all cases, I almost never need to leave the decorators out of my operation. If I do sometimes need to leave them out, I can manually adjust the selection later in a multitude of ways. I admit that this might be a personal preference issue, but I don't have reason to believe that my work differs from the average significantly in this instance.

In any case, please tell me if I need to adjust something else or otherwise, as I am not very experienced with treesitter yet.

upd: seeing the failing tests, will look into those later, feel free to post any advice for those

@kiyoon
Copy link
Collaborator

kiyoon commented Jul 18, 2024

I like this too. Can you make it the same for the class?

@kiyoon
Copy link
Collaborator

kiyoon commented Jul 18, 2024

But the tests are failing.. Do you know why? You don't have to care about the consistency tests because the functionality changed anyway

@kiyoon
Copy link
Collaborator

kiyoon commented Jul 18, 2024

Another problem is that if you just remove the textobject, then it won't grab when the function doesn't have decorators..

@feakuru
Copy link
Contributor Author

feakuru commented Jul 18, 2024

Can you make it the same for the class?

i'll take a look at the class queries, but i will make no promises since i am not very experienced with this 😄

@feakuru
Copy link
Contributor Author

feakuru commented Jul 23, 2024

i'll take a look at the class queries

so i did take a look and found that the query for class could be adjusted in the same way. did just that, seems to pass the tests

it won't grab when the function doesn't have decorators..

now it seemingly works for both cases, i basically just marked the decorated_definition as optional with a ? and voila 🎉 thanks for spotting this!

@feakuru feakuru changed the title make @function.outer for python always use the decorated version make @function.outer and @class.outerfor python always use the decorated version Jul 23, 2024
@feakuru feakuru changed the title make @function.outer and @class.outerfor python always use the decorated version make @function.outer and @class.outer for python always use the decorated version Jul 23, 2024
@kiyoon
Copy link
Collaborator

kiyoon commented Jul 24, 2024

Although I haven't tested it, I like the approach. Interestingly the consistency test still pass, and I thought it would fail. Maybe I didn't add the @function.outer tests?

@feakuru
Copy link
Contributor Author

feakuru commented Jul 24, 2024

Maybe I didn't add the @function.outer tests?

from what I understand from reading the consistency tests script, they only check a big loss.py example file which does not seem to contain any decorators, maybe that's the reason?

also thank you for the approval!

fyi: i think only maintainers can merge with failing status checks, and since failed loading of R syntax files is a known problem being solved elsewhere, please feel free to merge this at your convenience 🙂

@kiyoon
Copy link
Collaborator

kiyoon commented Jul 24, 2024

from what I understand from reading the consistency tests script, they only check a big loss.py example file which does not seem to contain any decorators

You're right! Thanks for looking into it.

I'll merge this when the query problem gets fixed

@kiyoon
Copy link
Collaborator

kiyoon commented Aug 2, 2024

Can you rebase it to master?

@feakuru
Copy link
Contributor Author

feakuru commented Aug 7, 2024

Can you rebase it to master?

done 🙃

@kiyoon kiyoon merged commit ca93cb2 into nvim-treesitter:master Aug 7, 2024
6 checks passed
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.

3 participants