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

subexpression marker ($ in '$(') is in scope 'punctuation.definition.variable' #132

Closed
msftrncs opened this issue Aug 14, 2018 · 6 comments · Fixed by #183
Closed

subexpression marker ($ in '$(') is in scope 'punctuation.definition.variable' #132

msftrncs opened this issue Aug 14, 2018 · 6 comments · Fixed by #183

Comments

@msftrncs
Copy link
Contributor

Environment

  • Editor and Version: VS Code: 1.26.0
  • Your primary theme: Dark+

Issue Description

The subexpression marker(the $ in '$(') is currently scoped in 'punctuation.definition.variable'. This causes no colorization in most schemes, such then cause colorization issues in double quoted text subexpressions.

Expected Behavior

I might think that it should be in a 'keyword' scope. To play around, I modified the VS Code powershell.tmLanguage.json file, in two places, to change the $ in '$(' to be scoped as 'keyword.other.subexpression.powershell'.

This seems to get the result with themes, causing it to colorize the same as the @ in '@(' or '@{' which also scope as a 'keyword'.

@vexx32
Copy link

vexx32 commented Jun 2, 2019

@TylerLeonhardt would be great to have something along these lines tbh, this is such a common thing in a lot of code I see and it can look really weird in strings at the moment. 😔

@msftrncs
Copy link
Contributor Author

msftrncs commented Jun 2, 2019

Related items: #106, #59

@vexx32, in a recent PR #156 commit, I have set the ${ and } of an interpolation (not a substatement) to scope as 'punctuation.section.embedded.begin/end.powershell'. Some editors/themes then have a highlight for that. See #59 above, I've been considering added a PR to get this handled... but I think I was running into the issue that substatement and interpolated substatement are handled in the same rule, would get same scopes, and so I would need to separate that out. That's what I did in PR #156, so I suppose that's inevitable. There was also some naming issues around some of those rules, which lead to confusion.

Would the punctuation.section.embedded scope be better than just keyword.other? I found the themes in VS Code supported the embedded scope for some other languages, but not PowerShell. Atom on the other hand appears to support the scope in all languages in the default theme.

It might be possible to apply both scopes to the interpolated version of the definition, then support in the theme can fallback to which ever is supported.

@vexx32
Copy link

vexx32 commented Jun 2, 2019

Hard to say from where I'm standing, I'm afraid. I'm not super familiar with how the different scopes are handled, but I'm painfully aware that a lot of themes have a bit of inconsistent support for many of the scope rules.

I guess it seems logical that if we can apply multiple that are sensible, at least one of them is likely to be covered by a given theme?

@msftrncs
Copy link
Contributor Author

msftrncs commented Jun 3, 2019

I've partially misspoke, I just found out that while VS Code's Monokai Dimmed does not highlight punctuation.section.embedded, the default Dark+/Light+ themes do.

My goal is still to try to post a small PR for this and related issues.

@msftrncs
Copy link
Contributor Author

msftrncs commented Jun 4, 2019

Disappointingly ATOM does not support specifying multiple scopes. This means that the tests in this project will fail since they are based on ATOM. :(

@msftrncs
Copy link
Contributor Author

I think I found a work-around for the ATOM issue.

VS Code:
image

ATOM:
image

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

Successfully merging a pull request may close this issue.

2 participants