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

Variable colors are prioritized over hashtable colors for keys in hashtables #182

Open
MartinGC94 opened this issue Aug 14, 2019 · 4 comments

Comments

@MartinGC94
Copy link

Keys inside hashtables use the "variable.*" scope over the more specific "meta.hashtable.assignment" scope, this means you can't give keys inside hashtables a custom color that is different from variables.

HashtableKeyProblem

(Screenshot is from VS code using the "Inspect TM scopes" feature)

@msftrncs
Copy link
Contributor

msftrncs commented Aug 15, 2019

meta.hashtable.assignment is less specific. It covers the entire assignment, from the beginning of the key name to the end of the value. meta scopes are not intended for use by themes.

A better scope for bareword key names is entity.name.variable.property or entity.name.variable.hashkey, as that is inline with what other grammars are using.

These are from my PR #156 and my custom theme in VS Code:
image
image
image

There are further things wrong with the hashtable portion of the grammar. A key of a hashtable is actually a non-expandable argument (err operand or identifier actually), which can include unary expressions, and these are not currently correctly handled, as they lose the assignment meta scope. (My PR is currently worse, since its a syntax scoping grammar instead of a keyword scoping grammar, and its an area I am still working on.)

PowerShell ISE/PowerShell AST/Tokenizer treats the bareword key name as 'MemberName', which is the equivalent of property.

@MartinGC94
Copy link
Author

Isn't this line the regex used for meta.hashtable.assignment?

<string>\b((?:\'|\")?)(\w+)((?:\'|\")?)(?:\s+)?(=)(?:\s+)?</string>

As far as I can tell that only captures up to the "=" part and when I use the inspect TM scopes feature to check out the value part of the statement it seems to confirm this.
It's good to know that meta scopes aren't meant to be used by themes, but I just wanted to be able to change it from my personal settings file, I don't plan on making any public themes myself.

It's good that your PR fixes the hashtable keys, but I'm not a fan of making class methods like: [convert]::ToBase64String() share the same scope as cmdlets/functions.

@msftrncs
Copy link
Contributor

That's the line, but it also only works for bareword/indentifier situations, because the regex is syntactically incorrect to capture the quotes, and incorrectly allows identifiers starting with numeric digits.

The meta.hashtable.assignment should technically wrap the entire assignment expression, but that takes a significant more complex rule set to do that yet. Also, additional meta's such as meta.hashtable.key-expression and 'meta.hashtable.value-expression` would also be implemented.

The intention of meta scopes was to aid tools, such as for region selection, or, for instance, meta.embedded allows the editor to offer code completion suggestions in embedded areas where it wouldn't have otherwise been obvious to make them available because they were considered 'not code' (such as inside a string interpolated subexpression "$(cmdlet-here -parameter value)")

The complexities with the methods and functions/cmdlets sharing the same scope comes from the fact that PowerShell does too many things. 😄 Themes don't expect to have to handle that many different ideas. One option is to add a higher precedence more specific scope, but I am finding some issue with that, in that Atom doesn't support that yet, that I have heard. I need to prove to Atom that TextMate supports it, but I have some ideas for workarounds involving more complicated rules.

@msftrncs
Copy link
Contributor

In PR #156 I did use slightly different scopes between methods and functions, entity.name.function and entity.name.function.method.

This way a normal theme will catch them both with entity.name.function but a more specialized theme can catch the extra method. Languages such as C# or JS do not bother with that since all functions are technically methods in those languages.

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

2 participants