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

feat: improve scope detection and calculation #805

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Danielkonge
Copy link
Contributor

@Danielkonge Danielkonge commented Dec 21, 2023

This uses the exact cursor position to find the treesitter node used when finding the scope, which improves scope detection in Python, Lua and presumably some other languages too.

New behavior:

Screenshot 2023-12-21 at 17 47 25 Screenshot 2023-12-21 at 17 47 40 Screenshot 2023-12-21 at 18 06 54 Screenshot 2023-12-21 at 18 07 12

Old wrong behavior that the above fixes:

Screenshot 2023-12-21 at 18 09 44 Screenshot 2023-12-21 at 18 09 10

Importantly, it doesn't change the language_for_range calculation, which from what I can tell is the reason the other range was starting at column 0 of the current row.

I also added an early exit to the scope calculation if the node didn't change (TSNode:equal(TSNode) tests that the nodes are in the same tree and the ids are equal), so we can skip the slightly slower calculations after that step (joining and going through tables). This should slightly speed up the calculation in a lot of cases (given all the auto commands used), but it is less important than the other change.

This should close #799.

@lukas-reineke
Copy link
Owner

Importantly, it doesn't change the language_for_range calculation, which from what I can tell is the reason the other range was starting at column 0 of the current row.

This is not the only reason. This is also needed to filter out single line scopes. There were a lot of other edge cases with this logic, I should have written tests for this..

In principle, I'm fine with the change, but it needs logic to filter single line scopes. And a lot of testing.

@Danielkonge
Copy link
Contributor Author

Just to make sure that I understand how you would want it to work.

My update gives behavior like this, since the scope is inside the function:

Screenshot 2023-12-27 at 16 14 30

The master branch gives behavior like this:
Screenshot 2023-12-27 at 16 15 03

Do you want an option to keep the old behavior? E.g., something like one_line_scope (maybe with a better name)? Because I would at least prefer to have an option actually underlines even for one line scopes. (I could underline the exact scope range in those cases too then.)

When I better understand the behavior you would want I can update this, and then I can add some tests after you have looked at it. (I would like to know if the code makes sense to you before adding the tests.)

@lukas-reineke
Copy link
Owner

Do you want an option to keep the old behavior?

The old behavior should be the default. I don't really want to add another option, maybe show_exact_scope can also enable single line scopes with just underline.

@Danielkonge
Copy link
Contributor Author

Danielkonge commented Jan 2, 2024

The old behavior should be the default. I don't really want to add another option, maybe show_exact_scope can also enable single line scopes with just underline.

I have updated the code now so that the old behavior for one line scopes is the default and we only highlight one line scopes if show_exact_scope is true.

The code still fixes the highlighting issue described in #799 for show_exact_scope both true and false.

Does this look good to you? Or do you want to change anything else?

If this looks good, I can try to add some tests too.

@sett-dozee

This comment was marked as off-topic.

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.

[Question] Python scopes are detected in the first line (header) for top-level scopes but not nested scopes.
4 participants