-
Notifications
You must be signed in to change notification settings - Fork 18
Highlight fixes #241
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
Conversation
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
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 rescript-lang
repo..
The parameter start
and end_
is highlighted as @property.rescript
. Wouldn't @variable.parameter
be better?
Js.Array2.slice(~start=0, ~end_=Js.Array2.length(moduleRoute) - 1)
"value"
is highlighted as @variable.member
. It should be @string
?
let version = (evt->ReactEvent.Form.target)["value"]
slug
is a value, I think it should just be @variable
let rehypePlugins = [Rehype.WithOptions([Plugin(Rehype.slug), SlugOption({prefix: slugPrefix ++ "-"})])]->Some // ^ slug is highlighted as variable.member
moduleName
inside template string is highlighted as @string
let pathModule = Path.join([dir, version, `${moduleName}.json`])
row
, column
and shrotMsg
is highlighted as @variable.member
but not row
in Api.LocMsg.row
. Should it be highlighted? I really don't know
let {Api.LocMsg.row: row, column, shortMsg} = locMsg // ^ not highligted
The parameter
start
andend_
is highlighted as@property.rescript
. Wouldn't@variable.parameter
be better?Js.Array2.slice(~start=0, ~end_=Js.Array2.length(moduleRoute) - 1)
parameter
is usually used for the parameter themselves when declaring the arguments of a function. Here, the labels are the names of the parameters. In other words, in your example, 0
is more of a variable.parameter
than ~start
.
My reasoning on using property
is that in the Neovim docs is defined as "The key in a key/value pair", which made sense here.
However, I can see how it's not an exact fit.
In OCaml they use @label
. It is not a good fit based solely on the Neovim docs description, which is:
"GOTO and other labels (e.g. label: in C), including heredoc labels"
However, label
is a whole different concept in OCaml and these are indeed labeled arguments, so it can work too.
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 parameter
, property
, or label
are fine.
"value"
is highlighted as@variable.member
. It should be@string
?let version = (evt->ReactEvent.Form.target)["value"]
Yeah, that should be @string
.
slug
is a value, I think it should just be@variable
let rehypePlugins = [Rehype.WithOptions([Plugin(Rehype.slug), SlugOption({prefix: slugPrefix ++ "-"})])]->Some // ^ slug is highlighted as variable.member
My thinking is that it's the member
of a module... but perhaps it's best to use member
only for record fields.
Indeed, in OCaml they use @variable
. I'll change it.
moduleName
inside template string is highlighted as@string
let pathModule = Path.join([dir, version, `${moduleName}.json`])
Yeah that's wrong.
row
,column
andshrotMsg
is highlighted as@variable.member
but notrow
inApi.LocMsg.row
. Should it be highlighted? I really don't knowlet {Api.LocMsg.row: row, column, shortMsg} = locMsg // ^ not highligted
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 variable
. (That's what the javascript highlights do.) But again, I like OCaml's approach.
parameter
is usually used for the parameter themselves when declaring the arguments of a function. Here, the labels are the names of the parameters. In other words, in your example,0
is more of avariable.parameter
than~start
.
I agree with @aspeddro; for other languages that have labeled arguments (e.g. Python) we highlight the labels as @variable.parameter
still. After all, the label is the name of the parameter and the value on the left is the actual passed-in argument, which imo makes e.g. ~start
more of a parameter than 0
(the actual argument)
queries/rescript/highlights.scm
Outdated
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.
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 @punctuation.delimiter
highlights for ":"
and @punctuation.bracket
for the "<"
and ">"
of template types? (You can just put those last ones broadly at the top and then the operator highlights will take higher precedence as long as they come after the other queries
queries/rescript/highlights.scm
Outdated
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
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 @variable
overrides @variable.parameter
for "cmp":
let comparable = (type key, ~cmp) => { module N = MakeComparable({ type t = key let cmp = cmp }) module(N: Comparable with type t = key) }
Also I think maybe the variable highlight capture should be moved higher in the file (or parameters should be lower?). In the following snippet
@variable
overrides@variable.parameter
for "cmp":let comparable = (type key, ~cmp) => { module N = MakeComparable({ type t = key let cmp = cmp }) module(N: Comparable with type t = key) }
You mean the mp
on the first line? It works for me right now. Maybe it was broken and I fixed it with the changes I made while you were commenting. Could you try it again?
You mean the
mp
on the first line? It works for me right now. Maybe it was broken and I fixed it with the changes I made while you were commenting. Could you try it again?
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 🎉
Uh oh!
There was an error while loading. Please reload this page.
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: