Python highlights to incl. types, parameters, etc. #2457
Conversation
* Better handling of @type highlighting * Parameters highlighted with @variable.parameter * Made @constructor more specific to highlight on call * Decorator indentifier highlighted as part of @function * cls and self treated as @variable.builtin Re-ordering of some statements to give a better priority. This takes some ideas from the neovim implementation.
| ((identifier) @constructor | ||
| (#match? @constructor "^[A-Z]")) | ||
| (type (identifier) @type) | ||
| (type (subscript (identifier) @type)) ; only one deep... |
There was a problem hiding this comment.
By the way, if anyone knows how to make this work for arbitrarily nested values, please let me know. It currently works for something like dict[str, int], but if it is any deeper dict[str, list[int]], the inner identifiers no longer get treated as @type (falls back to @variable I believe).
There was a problem hiding this comment.
The tree-sitter query language doesn't currently support specifying arbitrary nesting. tree-sitter/tree-sitter#880
In nvim-treesitter I've seen this be addressed by having some large number (~10) of stanzas for increasing depths but it's not very pretty and I think it can make the query analysis time increase potentially noticeably (and a high query analysis time will make helix seem to hang while it loads the parser and queries).
There was a problem hiding this comment.
I see thanks. I just realised I have to also handle the binary operator |. Do you think something like this would be an OK compromise:
; Handle subscript and | binary operator nesting 3 levels deep
(type
(_ (identifier)? @type
(_ (identifier)? @type
(_ (identifier)? @type))))There was a problem hiding this comment.
Implemented the above in new commit, but 4 levels deep.
There was a problem hiding this comment.
Do you have some code samples I could play around with for these cases? I have an idea for avoiding nesting but I'm not sure it'll work out.
There was a problem hiding this comment.
This is the exact opposite of how the neovim queries work.
Yeah all implementations follow this order except Neovim. I think the underlying engine is still the same but this behavior comes from how they interact with the queries.
There was a problem hiding this comment.
Hi @paul-scott , thank you so much for the thoughts and conversation!
On datetime.datetime coloring
I agree with you about number 3 (datetime.datetime). Highlighting the whole thing is preferable and I don't see a way to separate them either.
On Merging Efforts
I'm definitely up for merging our two efforts, but I'm not entirely sure how to do that. Do I need to fork your forked repo and then create a PR into yours and then you create a PR into helix? Seems super weird. I've only really used git in a corporate environment where I could just make branches directly.
On str, int functions
I agree about calling str, int, etc type.builtins. It might make sense to do the same for Union, Optional, and the others from the typing module as well. I'm not sure.
On coloring of None
I disagree about the usage of None. I understand your distinction about None being a special alias for type(None), but I doubt that the large majority of python users know about that exception... or that it really matters in practice. Consider this example, which is a pretty common idiom for mutable default arguments:
I think that the Optional is clearly easier to visually distinguish and I think that having the typed None green would make it easier to read as well, although, probably still not as easy as Optional. No worries if you remain unconvinced though! This is a minor point. And this screenshot is from VSCode again, so you are clearly in good company! I just personally find it kind of jarring to have a blue None on both sides of the equal sign.
On treesitter priority
Geesh, I've been all over the treesitter documentation and I couldn't find anything about the priority order! I don't know if I just missed it or if it's not there. Thank you two for the pointer! I've only started learning about treesitter at all a couple days ago.
On Constructors...
I'm not sure about the @constructor usefulness either. Seems weird to me.
Plan forward
We figure out how to get the best of both worlds and get world class python syntax highlighting in Helix!
There was a problem hiding this comment.
Perhaps it would be best if we work on your current PR. I can create a PR on your fork, and then if / when you merge it it should show up in the upstream PR. That effort can focus on fixing type highlighting and the additional control keyword stuff you implemented.
I can create a fresh separate PR for the additional non-type stuff I have here. If I can work out how I might mark this one as a draft.
I'll take a look and let you know if this makes some sense.
On str, int functions
There is a question of where to draw the line with builtin types / builtins in general. I'd say since Optional and Union are not in the prelude but in a module it would be better to leave them as regular types. The catch-all that regexes against things that start with a capital but that aren't a constant should deal with this fine.
On coloring of None
It should be possible to do as you prefer (in type hints, not type alias) for None, at least to a nesting depth n. Note that as it isn't an identifier it requires special treatment so will require extra (perhaps negligible?) overhead to implement. I'm on the fence with this one.
On Constructors...
Perhaps it is best to label things accurately if we can and leave it up to the themes to work out how to best visualise it. I guess it is easy enough to change a theme if you want to make constructor definitions / calls to look like functions and / or types.
There was a problem hiding this comment.
@paul-scott , I gave you contribute access to my helix fork. We can just work on branches if you want. It makes the most sense to me to work together and update all the python highlighting issues we can and then create one PR, but I'm good either way.
There was a problem hiding this comment.
@Zeddicus414 you'll probably see the PR on your fork before this, but just in case letting you know I've implemented all the changes I was interested in.
|
Combined efforts to implement these changes in #2541 instead. Closing. |
@typehighlighting@variable.parameter@constructormore specific to highlight on call@functionclsandselftreated as@variable.builtinRe-ordering of some statements to give a better priority.
This takes some ideas from the neovim implementation.
Looks like it might be relevant to #2427 and is probably an alternative to #2451.
Converted to draft to work on a combined effort on type highlighting with the above PR.