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

Multiple fixes #94

Merged
merged 63 commits into from
May 21, 2018
Merged

Multiple fixes #94

merged 63 commits into from
May 21, 2018

Conversation

omniomi
Copy link
Collaborator

@omniomi omniomi commented May 10, 2018

General changes:

  • Double quoted strings are now a repository item (required for other fixes.)

Scope changes:

  • Variable tokens ($,@) now scoped keyword.other.variable.definition.powershell. While they should be punctuation.definition.variable not many themes cover punctuation.definition.* due to there not being as many token based languages.

  • Variables are now scoped variable.other.readwrite.powershell.

  • Parentheses, brackets, and braces ((,),[,],{,}) moved to Punctuation.section.* scope.

  • Types now scoped storage.type.powershell.

Fixes:

  • Attributes (like [cmdletbinding()] and [validatepattern()])

    • Fixed highlighting issues where certain characters would break document highlighting.
    • Fixed parameter highlighting where certain spacing would break line highlighting.
    • Numerous scoping changes.
  • Type declarations containing digits (like [int32]) no longer break highlighting.

attributes

@omniomi
Copy link
Collaborator Author

omniomi commented May 10, 2018

Fixes #75
Fixes #74
Fixes #73
Fixes #71
Fixes #65
Fixes #64
Fixes #56
Fixes #51
Fixes #37
Fixes #38
Fixes #28
Fixes #27
Fixes #26
Fixes #17
Fixes #15
Fixes #14
Fixes #88

@omniomi
Copy link
Collaborator Author

omniomi commented May 10, 2018

Just noticed, there's one edge case that breaks highlighting that I still need to figure out...

[attribute(blah blah blah)
]

breaks highlighting even with a \s* between the \) and \] in the pattern for the end captures.

EDIT: fixed in 350afda

@omniomi omniomi changed the title Multiple fixes Multiple fixes [WIP] May 10, 2018
@kborowinski
Copy link
Contributor

kborowinski commented May 10, 2018

This's really nice job @omniomi ! I have found small edge case in ValidateScript attribute with complex scriptblock when the else keyword is not highlighted:

image

BTW could you have a look on enum highlighting since you are on a roll ;)

image

@kborowinski
Copy link
Contributor

kborowinski commented May 10, 2018

@tylerl0706 I have noticed that PowerShellSyntax.tmLanguage didn't change in couple of latest VSCode Insiders builds. Should we ping someone at VSCode to update it?

@omniomi
Copy link
Collaborator Author

omniomi commented May 10, 2018

@kborowinski can you post that snippet as plain text so I can copy/paste for testing please.

@kborowinski
Copy link
Contributor

@omniomi Yep, there you go:

function Test-Highlighting {
    param(
        [ValidateScript({
            if (($_ -gt ($upperLimit = ((Get-Date) - (Get-Date '1601-01-01')).Days)) -or ($_ -lt 0)) {
                throw ("The specified value '{0}' is outside allowed range (0 to {1})" -f $_, $upperLimit)
            } else {
                $true
            }
        })]
        [Int]$InactivityDays = 0
    )
    $InactivityDays
}

@omniomi
Copy link
Collaborator Author

omniomi commented May 10, 2018

@kborowinski it's treating the closing brace of the if statement as the close of the scriptblock which puts "else" into meta.attribute.powershell which gives it no definition.

The correct solution would be to properly define if/else/elseif statements not just their keywords... the quick and dirty solution is to include $self inside the attribute definition... I'm inclined to do it properly so it will take a bit.

ifelse-1

ifelse-2

@kborowinski
Copy link
Contributor

kborowinski commented May 10, 2018

@omniomi So enclosing the whole script in brackets should theoretically force parser to treat it as one block? Let me do quick check.

Edit: Yes it does work
image

BTW, sorry for naive question, but how you get these nice popups with token details?

@omniomi
Copy link
Collaborator Author

omniomi commented May 10, 2018

@kborowinski correct, but you should not need to do that. So I'll add it to the list of things to fix.

CTRL/Cmd+P> Developer: Inspect TM Scopes

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented May 10, 2018

@kborowinski I'll check to see what vscode's process is exactly.


build_script:
- ps: Set-Location (Join-Path $env:APPVEYOR_BUILD_FOLDER '\tools\')
- ps: BuildBanner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to address this now but I want to mention that I'm hoping to try and make the build.ps1 generic enough that we can use just build.ps1 commands in the appveyor.yml.

Then we can easily drop in an appveyor.yml into any repo that follows the build.ps1 concept and we'll have CI :)

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 it!

LGTM 🎉

@vors
Copy link
Collaborator

vors commented May 21, 2018

🎉

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 this pull request may close these issues.

None yet

4 participants