-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
python: Refine highlighting #21389
base: main
Are you sure you want to change the base?
python: Refine highlighting #21389
Conversation
; Any identifiers within a type are in practice going to be type identifiers; this nested scan | ||
; is currently the only way to support cases such as `type MyList[T] = T | list[T]` | ||
(type (_ (identifier) @type)) | ||
(type (_ (_ (identifier) @type))) |
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.
@osiewicz Currently this PR is still in draft, but already in advance I feel that this approach here is incredibly silly. I am not sure if it is possible to apply a rule for an arbitrarily nested case such as this?
type AnyTasks[T: ExecutableTask | Task] = T | _TaskList[T] | list[T | _TaskList[T]] | None
I feel like the problem boils down to not getting adequate output from the parser so we're having to do more complicated stuff in the highlighter.
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.
Hmm. I'll revisit your PR tomorrow, but off the top of my head that's not going to be easy to pull off cleanly. Tree-sitter doesn't support matches of arbitrary depth.
Besides, I think we should fall back on semantic highlighting for cases like these. While in principle the only valid kind of identifier in that position is a type, users code might be invalid. Thus, if possible, I'd rather have us tackle semantic tokens instead of handling these cases at the tree-sitter level.
Don't get me wrong, I really appreciate your recent efforts on Python highlighting. It's just that this PR runs into a wall that should be brought down, not chipped away at :P
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.
Hm, neither Pyright nor pylsp support semantic tokens at the moment.. That's a huge bummer.
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.
Yeah I agree this is too big of a hack around the actual issue so I think I'll remove this part for now and maybe open an issue for it instead.
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.
With the other adjustments my python project is 99.8% correctly highlighted though so its a good experience compared to a month ago already :)
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.
Also regarding semantic tokens I believe basedpyright already has support for this https://docs.basedpyright.com/latest/benefits-over-pyright/pylance-features/.
This would make it another big feature in support of basedpyright at this stage.
WIP. Will test and add more.
Release Notes: