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.

Sign up for GitHub

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

Jump to bottom

Embedded subexpressions #183

Merged
merged 2 commits into from
Mar 17, 2021
Merged

Embedded subexpressions #183

merged 2 commits into from
Mar 17, 2021

Conversation

msftrncs
Copy link
Contributor

Improve embedded substatements, fix #132.

  • Rename #interpolatedStringContent to #subexpression.
  • Remove include #interpolation from #subexpression.
  • Change scopes in #interpolation to include meta.embedded, keyword.other was
    punctuation.definition.variable.
  • Change scope in root pattern for $( ), keyword.other was
    punctuation.definition.variable. (No tests are present for this)
  • Remove redundant includes from #subexpression and #interpolation.
  • Adjust test, of embedded substatements in double quoted strings.

Sample appearance

  • VS Code (before):
    image

  • VS Code:
    image

  • ATOM:
    image

@msftrncs
imrpove embedded subexpressions
d14162e 
rename #interpolatedStringContent to subexpression.
change subexpression scope to not include 'interpolation'.
change scopes for #interpolation to include embedded, keyword.other was
punctuation.definition.variable.
Change scope for regular `$()`, keyword.other was
punctuation.definition.variable.
removed redundant includes from #subexpression and #interpolation.
@msftrncs
Adjust test, of embedded substatements in double quoted strings.
84a64bc
@msftrncs msftrncs mentioned this pull request Mar 3, 2021
Closed
@andyleejordan
Copy link
Member

@msftrncs Is this PR superseded by #156, or should we merge it while trying to get that one in (since it appears to be a complete rewrite)?

@msftrncs
Copy link
Contributor Author

This was meant to be separate of #156, so that it could be benefitted from sooner. Its one of several PR's I made trying to bring in some trivial fixes quicker. This goes along with #173 and #175

@andyleejordan andyleejordan merged commit 0778739 into PowerShell:master Mar 17, 2021
@andyleejordan
Copy link
Member

Thanks @msftrncs! After evaluating the state of the tests, CI, and these three PRs, we went ahead and merged. What's the worst that could happen? 😁

@andyleejordan
Copy link
Member

I know it took a couple years but @msftrncs I wanted to let you know that GitHub Linguist just shipped which includes this and a few of your other fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt rjmholt approved these changes

@andyleejordan andyleejordan andyleejordan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

subexpression marker ($ in '$(') is in scope 'punctuation.definition.variable'
3 participants
@msftrncs @andyleejordan @rjmholt