Highlight fixes

[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:

• Highlights function calls
• Highlights module and record pattern members
• Highlights units as built-in constants
• Highlights labeled parameters

[aspeddro]

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)

Code

---

"value" is highlighted as @variable.member. It should be @string?

let version = (evt->ReactEvent.Form.target)["value"]

Code

---

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

Code

---

moduleName inside template string is highlighted as @string

let pathModule = Path.join([dir, version, `${moduleName}.json`])

Code

---

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     

Code

[Emilios1995]

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)

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.

Code

"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 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     

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.

[ribru17]

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.

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)

[ribru17]

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

[ribru17]

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)
}

[Emilios1995]

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?

[ribru17]

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

[Emilios1995]

@aspeddro I believe I've addressed all the feedback from you and @ribru17 (thanks btw!)

Do you think we can merge this?

[aspeddro]

Sorry for the delay. I'll review it this week!!