-
Notifications
You must be signed in to change notification settings - Fork 15
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
Highlight fixes #241
Highlight fixes #241
Conversation
|
||
; single parameter with no parens | ||
(function parameter: (value_identifier) @parameter) | ||
(function parameter: (value_identifier) @variable.parameter) |
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.
This and other similar changes conform to to Neovim's new tags with fallbacks. Meaning, if a certain theme specifies a color for @variable.parameter
, it will be used, but otherwise it will fall back to the color for @variable
.
b5bbe2f
to
8461c77
Compare
(module_identifier) @namespace | ||
(module_identifier) @module | ||
|
||
(member_expression (property_identifier) @variable.member) |
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.
This was @property
and was changed to @variable.member
. I looked at other languages and this is how it's usually handled: @property
is for places such as record definition when both the property and value are explicitly written, and member is for extracting a value out of that property. (which is why it belongs to the variable
group.)
Great job, leaving some comments and questions I had when testing highlighting in the The parameter Js.Array2.slice(~start=0, ~end_=Js.Array2.length(moduleRoute) - 1)
let version = (evt->ReactEvent.Form.target)["value"]
let rehypePlugins =
[Rehype.WithOptions([Plugin(Rehype.slug), SlugOption({prefix: slugPrefix ++ "-"})])]->Some
// ^ slug is highlighted as variable.member
let pathModule = Path.join([dir, version, `${moduleName}.json`])
let {Api.LocMsg.row: row, column, shortMsg} = locMsg
// ^ not highligted |
My reasoning on using However, I can see how it's not an exact fit. In OCaml they use What do you think? It's hard to debate what label fits better objectively, and it's hard to analyze which one looks better across themes. Honestly I think either
Yeah, that should be
My thinking is that it's the
Yeah that's wrong.
I like how OCaml handles it. Applied to this example, it would be: let {Api.LocMsg.row: row, column, shortMsg} = locMsg
// ^@variable.member
// ^@variable
// ^ @variable.member
// ^ @variable.member So regarding the row part, I definitely think it's the right highlight. For column and shortMsg I also like it, but we could also make them |
I agree with @aspeddro; for other languages that have labeled arguments (e.g. Python) we highlight the labels as |
queries/rescript/highlights.scm
Outdated
@@ -66,50 +91,83 @@ | |||
[ | |||
(parameter (value_identifier)) | |||
(labeled_parameter (value_identifier)) | |||
] @parameter | |||
] @variable.parameter |
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.
I think this was a bug before that I also had to fix when I changed the queries; it should be
(parameter (value_identifier) @parameter)
(labeled_parameter (value_identifier) @parameter)
otherwise the parameter highlight is too broad.
"private" | ||
"rec" | ||
"type" | ||
"and" | ||
"assert" | ||
"await" |
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.
Would you mind moving this one to @keyword.coroutine
?
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.
Sure, I moved both async
and await
Also would you mind giving |
queries/rescript/highlights.scm
Outdated
|
||
|
||
[ | ||
"as" |
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.
I think this one would fit more in @keyword.operator
queries/rescript/highlights.scm
Outdated
|
||
|
||
((unit) @constant.builtin | ||
(#set! "priority" 105)) |
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.
Usually in nvim-treesitter
we try to avoid priority metadata, I think if you just place this query below the @punctuation.bracket
query it should take precedence over it 👍
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.
I tried that but I'm not sure why it doesn't work.
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.
Ah must be because it is a less-specific capture; (unit ["(" ")"] @constant.builtin)
works for me
Also I think maybe the variable highlight capture should be moved higher in the file (or parameters should be lower?). In the following snippet let comparable = (type key, ~cmp) => {
module N = MakeComparable({
type t = key
let cmp = cmp
})
module(N: Comparable with type t = key)
} |
You mean the |
Nice thanks! It is fixed for me |
It's what's in the neovim docs right now
ef2a311
to
f81c39d
Compare
Sorry for the delay. I'll review it this week!! |
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.
Thanks a lot @Emilios1995 🎉
This PR contains some updates to existing highlights to conform to neovim's latest documentation, and also introduces a few missing ones.
The new features are: