Module names aren't highlighting
maco opened this issue · 17 comments
tree-sitter-elixir was pushed live on GitHub today 🎉 , but @the-mikedavis pointed out that module names aren't highlighting
Example: https://github.com/elixir-mint/mint/blob/1ea7731d921840ad75a47fa9415525c3d94b9980/mix.exs#L1-L2
CC @patrickt (who asked to be CC'd so he can push an updated grammar on GitHub's end when this is handled.)
Hey! Modules are highlighted correctly for me when running tree-sitter highlighter locally:
This is probably related to the theme GitHub uses? The modules get the type highlight token, should we be using something else?
From what I can tell it looks like these highlights aren't ending up syntax-highlighted to any color:
tree-sitter-elixir/queries/highlights.scm
Lines 65 to 69 in de3ec57
which is curious because the java queries for example use @type as well: https://github.com/tree-sitter/tree-sitter-java/blob/ed3a87f750b1d1d533f15ab93fef3e1f5a46e234/queries/highlights.scm#L22-L23, and I assume java is tree-sitter highlighted in github, and I see class names highlighted as I would expect (e.g. here)
Maybe it's just a precedence issue? Or maybe github also needs a tags.scm to identify classes/modules?
Without @type the module name doesn't get any highlighting token, so there should be no conflict either way 🤔
The module name gets the pl-smi class instead of pl-en (or pl-v).
Ah good find: seems like the problem isn't with the grammar/queries here but something in how GitHub is mapping the tree-sitter scopes to class names. I'm not sure how to get the ball rolling with GitHub support on this. Maybe we can re-use the contact mentioned in #2 (comment) (cc: @josevalim)?
Yeah, I don’t think @type is quite right here. I’m trying to figure this out (and will assemble some docs, hopefully).
The tests for Java tree-sitter highlighting are in this file and it confuses me because tokens annotated as type are highlighted with three different colors. This would only make sense if for some reason tree-sitter is not used for highlighting Java files.
Looking at similar tests for JavaScript and Ruby, it looks like modules/classes use the constructor token, which doesn't seem suitable in our case.
@jonatanklosko Correct: I just found out that tree-sitter-java’s highlights aren’t hitting prod traffic, so a better comparison is Python or Ruby in their respective repositories. Apologies for the confusion.
Is the mapping/theme a part of some open source project or is it something you maintain internally?
It’s closed-source. I’m dotting my i’s properly and checking to make sure I can give you the whole list without running awry of lawyers. But yes, @type maps to the pl-smi class, which doesn’t appear to be used. I would try the Right Fix here is for us to change our backend to recognize a new tag, constructor for the time being; I know it doesn’t exactly match the semantics of what modules mean in Elixir, but@module. This will take longer, though I’ll put the wheels in motion. In the meantime, I can deploy updated changes to highlights.scm basically instantly, so a quick fix might be suitable here if it’s bumming people out.
@jonatanklosko Actually, I think I can get that @module change in pretty fast. So change those @type to @module and I’ll coordinate on my backend.
Fantastic!
Okay, the changes required to recognize @module in syntax highlighting in our backend are coming in and should be deployed in the next fifteen minutes or so. Once #16 lands, I’ll bump the submodule in our backend, and deploy again. Thanks all for your patience—this is our first community-maintained syntax highlighter, I believe, so thanks for being our guinea pigs! 😅
Beautiful, thanks for all the help! I've just merged the change, so we should be good on this side :D
This is working now:
Awesome, thanks! 
The examples linked in the issue description are still the same, is that caching temporary?
@jonatanklosko Yeah, I think that’s memcached doing its thing. It’ll fall out of cache eventually, heh!
Perfect :D