Skip to content
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

Synxtax color entity.name.type not validated for [type] over storage.type scope #179

Open
PhilipHaglund opened this issue Jun 12, 2019 · 6 comments

Comments

@PhilipHaglund
Copy link

Environment

  • Editor and Version:
    Visual Studio Code
    Version: 1.35.0 (system setup)
    Commit: 553cfb2c2205db5f15f3ee8395bbd5cf066d357d
    Date: 2019-06-04T01:18:19.664Z
    Electron: 3.1.8
    Chrome: 66.0.3359.181
    Node.js: 10.2.0
    V8: 6.6.346.32
    OS: Windows_NT ia32 10.0.17763

  • Your primary theme: PowerShell ISE

Issue Description

The entity.name.type is not validated over storage.type when defined in settings.json nor in theme.json.
When defining a type, i.e. [string], and using a statement, i.e. function, they both use the same syntax color from the storage.type scope.
Is the entity.name.type scope not defined for types?

Screenshots

Example code. Pasted the tokenColorCustomization from Settings.json in the same file for example purposes.
image

[string] type
image

function statement
image

Expected Behavior

In PowerShell ISE the statements uses the Keyword token with color '#00008b' and the types uses the Type token with color '#008080'. Is there another way to distinguish these scopes or tokens that I've missed or is the powershell.tmLanguage.json missing something?

@JustinGrote
Copy link

JustinGrote commented Aug 13, 2019

I'm just wondering why storage.type.powershell overrides meta.function.powershell on the "function" keyword. I see that javascript does the same thing, but why does storage.type take precedence over meta.function? Meta.function is more specific and allows for the function keyword to be colorized separate from the generic storage.type.

EDIT: It's because meta is a special keyword that doesn't get a precedence, and refers to the entire function block, not the function keyword itself.

@JustinGrote
Copy link

JustinGrote commented Aug 13, 2019

Also entity.name.type refers to the name of the function, e.g. function MyFunction, not the "function" keyword itself.

image

I'm working on a grammar injection extension that will add a more specific "storage.type.function.powershell" to the function keyword, so you can theme it appropriately, until such time the new and improved grammar shown in a pull request becomes the primary option.

@msftrncs
Copy link
Contributor

@JustinGrote, in PowerShell, functions are really static methods (which also get the scope entity.name.function), not types, so entity.name.type would not be the right choice. The name of a PowerShell class would be an entity.name.type scope.

Somewhere people thought that function and string are both the same thing (type, though usually function ends up storage and string ends up support). Hence the JavaScript issue with var and const and class and function and the like are also considered type. var and const are actually storage.modifier, not type, and class/function is a keyword, not type. The thought, that the keyword is going to define a type should not result in the keyword being considered a 'type'.

I have spent a bunch of time addressing this in my own theme, specifically capturing the javascript and typescript scopes and forcing them to the same color as C# scopes.

Think about the following:
image
image

If class was also storage type, how would we differentiate it from the type references? Type references are not entity's so the entity.name scope is not valid. The entity is coming after, in this case, the method of the class is the entity.

This is all just food for thought.

@JustinGrote
Copy link

JustinGrote commented Aug 14, 2019

@msftrncs I think you got my response confused with @PhilipHaglund.

@PhilipHaglund is saying that the function keyword should be entity.name.type.

I agree with you that it should not, it should be storage.type.function.powershell. The function name itself should be entity.name.function.powershell, per my example, as well as keyword.declaration.function.powershell to be in line with Sublime Scope Naming conventions

image

@PhilipHaglund I have made a vscode extension that addresses this issue and properly formats the function so that you can theme it appropriately. It's an effective workaround until the EditorSyntax file gets revamped (looks like work has stalled on that front for now)

https://marketplace.visualstudio.com/items?itemName=justin-grote.better-powershell-syntax-highlighting

Install this and you'll now see it will format correctly.
image

@PhilipHaglund
Copy link
Author

Thanks @JustinGrote will check that out! :)
Since I've not very familiar with OSS, specifically with issues, should I close this one and mark as a workaround or leave it open?

@msftrncs
Copy link
Contributor

There is no reason to continue to support storage.type for keywords. Sublime's recommendation could really be improved by putting that reference in the keyword section. The preferred scope should be the section items appear in. Obviously many grammar's still do not apply the recommended scope selections suggestions, or we wouldn't be drowned in a sea of storage.type (JS and TS).

Furthermore, its surprising, as many times as I have looked at the Sublime document, I never noticed the abundant suggestion to use multiple scope selections, while the Atom editor's theme system doesn't support it. Themes in Atom have to match the entire selector (which they cannot do, because the space is a separator character in the selector expression). Its this reason that probably prevents grammars such as JS/TS from adopting the recommendation correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants