nushell / vscode-nushell-lang

A Nushell grammar for Visual Studio Code with IDE support
https://www.nushell.sh/
MIT License
122 stars 27 forks source link

Spread not highlighted correctly #173

Closed texastoland closed 9 months ago

texastoland commented 9 months ago

Looking at the grammar file I assume it just hasn't been added yet. Minimal repro:

[...$stuff]
{ ...$stuff }
fdncred commented 9 months ago

Are these the only two variations? It would be good to have an example of each variation so they can be validated.

I can see that { ...$rest } seems correct and is called variable.other.nushell but [...$rest] isn't recognized properly and it's either string.bare.nushell or keyword.control.nushell depending on where you click on the text.

@glcraft Thoughts on this?

texastoland commented 9 months ago

I can see that { ...$rest } seems correct

They highlight identically for me. The first .. is highlighted like match then the rest like a bare identifier.

fdncred commented 9 months ago

Not identical for me. image

fdncred commented 9 months ago

Now that I look closer, neither of them are recognized correctly but one is closer than the other. image

You can see this menu with ctrl+shift+p Developer: Inspect Editor Tokens and Scope

texastoland commented 9 months ago

It looks like our themes target different scopes. I confirmed they both change scopes inside the spread. Code 2024-02-15 at 14 59 47@2x

fdncred commented 9 months ago

@texastoland Are you on the latest version of the extension v1.8.0?

texastoland commented 9 months ago

I am. You should see the same behavior if you inspect the operator. Your theme is just highlighting fewer scopes.

The right fix looks adapting (and renaming) the ranges regex to permit a third dot:

https://github.com/nushell/vscode-nushell-lang/blob/bd9d19fd83a010af6b007def456d59138d44a59f/syntaxes/nushell.tmLanguage.json#L186-L189

To: "\\.\\.[<.]?" (|: appears to be a mistake). There are other operator groups but they're surrounded by spaces. PR?

fdncred commented 9 months ago

What theme are you using? I'd like to be able to see the difference.

texastoland commented 9 months ago

Oh just checked my other themes but they're like yours. Screenshot using Tinacious Design.

fdncred commented 9 months ago

That range fix may be helpful but I don't think it's the entire solution. I don't think we should be getting multiple primary textmate scopes as I move my cursor from left to right. I get these primary scopes.

fdncred commented 9 months ago

Yup, I see it in Tinacious. You're right, tinacious is coloring more of the scopes. image

texastoland commented 9 months ago
[ ...$rest ]
{ ...$rest }

From my minimal example here's the (unfixed) scopes I see:

[] {}
.. keyword.control.nushell keyword.control.nushell
meta.table.nushell meta.expression.braced.nushell
source.nushell source.nushell
.$rest string.bare.nushell variable.other.nushell meta.command.nushell
meta.table.nushell meta.expression.braced.nushell
source.nushell source.nushell

It looks my patch would end up interpreting ... as an operator inside a closure instead of spreading a record. The alternative would be to include a new spread expression in each of the table and braced groups:

https://github.com/nushell/vscode-nushell-lang/blob/bd9d19fd83a010af6b007def456d59138d44a59f/syntaxes/nushell.tmLanguage.json#L215-L221

https://github.com/nushell/vscode-nushell-lang/blob/bd9d19fd83a010af6b007def456d59138d44a59f/syntaxes/nushell.tmLanguage.json#L516-L565

fdncred commented 9 months ago

ya, I think we should have a new spread_expression to accomodate this new syntax.

texastoland commented 9 months ago

I'll PR tomorrow then and include a table with the new scopes 👌🏼

texastoland commented 9 months ago

Deleted a previous comment because I was mistaken and amended my PR 🪚