PowerShell / EditorSyntax

PowerShell syntax highlighting for editors (VS Code, Atom, SublimeText, TextMate, etc.) and GitHub!
MIT License
133 stars 45 forks source link

Dollar sign '$' should be included as part of the variable token #23

Closed daviwil closed 6 years ago

daviwil commented 8 years ago

The dollar sign $ should be included as part of the variable token because editors like VS Code use the syntax tokens to determine what text to select when the user double-clicks in the current file. Currently if the user double-clicks in a variable like $myVar, only the myVar portion will be selected since the $ is treated as a separate symbol.

daviwil commented 8 years ago

Originally reported by @deshiknaves with issue https://github.com/PowerShell/vscode-powershell/issues/240.

MacKopes1 commented 7 years ago

I would like this fixed as well. Curious if there is any progress/status on it.

SillyCode commented 7 years ago

Would very much like this too by way of default.

Eventually, ended up with editing the "editor.wordSeparators" setting for PHP only. As per documentation suggestion. Using: "Preferences: Configure language specific settings" as the command for selecting PHP.

gravejester commented 7 years ago

Are we sure this is what we really want? Some of the guides I have seen regarding scope names suggests that the $ should be scoped as punctuation.definition.variable I tried a test where I renamed this scope to variable.punctuation as this would mean that both the $ and the variable name have the same root scope. Double-clicking in vscode still only highlighted the variable name.

I then tried to apply a meta.variable scope to the whole token ($ as well as the variable name), with the same results.

Checking in ISE this is the exact same behaviour I see there. The name of the variable is highlighted on double-click, not the $.

MacKopes1 commented 7 years ago

Yep, you are correct that this is the same behavior as in PowerShell ISE (which is ALSO terrible behavior). I STILL default to using the very old PowerShell GUI Editor because it DOES have the correct behavior of including the $ when double clicking. It's a huge burden on speed to not be able to quickly copy and paste variables including the $

gravejester commented 7 years ago

My only concern is that it won't work even if we scope the $ the same as the variable name. @daviwil do you know for certain that the editor is using the scope names for this? If not, is there any other way we can achieve this? Perhaps as a setting so that people can choose if they want this behavior or not?

gravejester commented 7 years ago

As mentioned above, adding the following to the settings works:

"[powershell]": {
    "editor.wordSeparators": "`~!@#%^&*()-=+[{]}\\|;:'\",.<>/?"
}
daviwil commented 7 years ago

@gravejester hmmm... If this is driven by editor.wordSeparators, then maybe the syntax def has nothing to do with it. Let me see if there's some way I can make this happen via the PS extension.

daviwil commented 7 years ago

Filed an issue with the VS Code team to see if there's a way I can override that setting.

Also just found this issue at the vscode-PowerShell repo where someone also asked for the dollar sign to be colored the same as the variable name.

gravejester commented 7 years ago

Yeah, I have mixed feelings about that. In the grammar I'm much more concerned with the tokens getting "proper" scope names, and leave it up the the different themes how these should be colored. I'd rather not cheat by using the wrong scope names just so that a token can get a particular color in the default theme. Thoughts?

daviwil commented 7 years ago

Sounds fine to me to have accurate tokens. The ISE theme can color the token consistently with the ISE :)

omniomi commented 6 years ago

Closing as the discussion appears to have reached a conclusions and an issue was opened for VS Code. @gravejester is also correct about punctuation.definition.variable. See: https://www.sublimetext.com/docs/3/scope_naming.html#variable

We can add this as a line item when we review scopes in general if someone wants to make a case for breaking convention.

vors commented 6 years ago

@lzybkr made a case in https://github.com/SublimeText/PowerShell/issues/147

The default coloring of variables in VSCode is hard to read because the sigil is colored, but not the variable name. This puts too much emphasis on the sigil and not the name.

I prefer having the sigil+variable colored, but not coloring anything would be better than the default.

Here is an example:

image

omniomi commented 6 years ago

Currently the sigil is scoped keyword.other.variable.definition.powershell while the variable name is variable.other.readwrite.powershell. The scope for the sigil is actually wrong (intentionally) so that it will be colored. It should be punctuation.definition.variable.powershell but very few themes cover the punctuation scope let alone the punctuation.defintition.variable scope.

If we set it to punctuation.definition.variable.poweshell right now it would inherit from whatever it's inside when a theme doesn't cover it so $String = "This is a string containing a $Variable" would highlight the $ the same colour as the string while doing the word Variable using variable.other... which is likely covered in the theme.

I am personally not inclined to scope the sigil the same as the variable name because I when designing themes want the option to target it specifically; However, what we could do is make the whole variable variable.other.readwrite.powershell including the sigil and then target the sigil as punctuation.definition.variable.poweshell. That would mean if the theme has punctuation.definition.variable it will use it but if not the parent would be variable.other.readwrite and it would fall back to coloring it the same as the name. Ie:

$Variable
^ punctuation.definition.variable.poweshell
^^^^^^^^^ variable.other.readwrite.powershell

with overlap.

vors commented 6 years ago

How does the double-click select behave with overlap?

I like the proposition so far, but it would be awesome to see it visually with 1 or more default themes.

lzybkr commented 6 years ago

The proposal sounds reasonable.

I'm not a fan of busy themes so it feels like it's complicating the syntax unnecessarily, but as long as it resolves the issue, then I won't complain.