Skip to content

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

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

Closed
daviwil opened this issue Sep 12, 2016 · 16 comments · Fixed by #118
Closed

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

daviwil opened this issue Sep 12, 2016 · 16 comments · Fixed by #118

Comments

@daviwil
Copy link
Contributor

daviwil commented Sep 12, 2016

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 daviwil added the bug label Sep 12, 2016
@daviwil
Copy link
Contributor Author

daviwil commented Sep 12, 2016

Originally reported by @deshiknaves with issue PowerShell/vscode-powershell#240.

@MacKopes1
Copy link

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

@SillyCode
Copy link

SillyCode commented Jul 31, 2017

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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

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

@daviwil
Copy link
Contributor Author

daviwil commented Sep 1, 2017

@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
Copy link
Contributor Author

daviwil commented Sep 1, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

daviwil commented Sep 1, 2017

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

@gravejester gravejester added discussion and removed bug labels Sep 4, 2017
@omniomi
Copy link
Contributor

omniomi commented May 21, 2018

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.

@omniomi omniomi closed this as completed May 21, 2018
@vors
Copy link
Contributor

vors commented May 26, 2018

@lzybkr made a case in SublimeText/PowerShell#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
Copy link
Contributor

omniomi commented May 26, 2018

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
Copy link
Contributor

vors commented May 26, 2018

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
Copy link

lzybkr commented May 29, 2018

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.

omniomi added a commit to omniomi/EditorSyntax that referenced this issue Jun 9, 2018
omniomi added a commit to omniomi/EditorSyntax that referenced this issue Jun 9, 2018
@omniomi omniomi mentioned this issue Jun 9, 2018
TylerLeonhardt pushed a commit that referenced this issue Jun 11, 2018
* rescope statement separator per #114

* rescope everything after --% #113

* rescope string punctuation per #110

* include sigil in variable scope per #23

* rescope variable member per #107

* rescope variable sigil per #23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants