Skip to content

Conversation

@msftrncs
Copy link
Contributor

@msftrncs msftrncs commented Sep 5, 2019

PR Summary

Flag the switch statement condition clause default token as a keyword
token to facilitate (PSReadLine) highlighting it as a keyword. Closes #10470.

This change is to treat the default clause keyword as a full keyword token, and as such token based highlighters will see it as a keyword.

PR Context

Flagging the default condition clause token of the switch statement as a keyword will facilitate highlighting of the keyword to better contrast against other bareword string literal conditions.

A simple (partial) example:

# return simple element value (need to check for Boolean datatype, and process value accordingly)
switch ($node.Name) {
    true { $true; continue } # return a Boolean TRUE value
    false { $false; continue } # return a Boolean FALSE value
    default { $node.Value } # return the element value
}

Highlighting of the default keyword in the above example makes it easier to distinguish the special condition clause from the other bareword conditions.

PR Checklist

@msftrncs msftrncs requested a review from daxian-dbw as a code owner September 5, 2019 06:21
@msftrncs
Copy link
Contributor Author

msftrncs commented Sep 5, 2019

I may need guidance on tests, I have not yet ventured in to any of the tests so far.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 5, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.4 milestone Sep 5, 2019
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 5, 2019

I do not find any tests for keywords.

@msftrncs
Copy link
Contributor Author

As for tests, it looks like, adding to test\powershell\language\parser\parsing.tests.ps1, some ParseInput expressions could be constructed to test the tokenizer results of a sample switch statement. I am referencing the ternary operator parsing tests that @daxian-dbw added.

@msftrncs msftrncs force-pushed the SwitchDefaultAsKeyword branch from 19bc872 to 6240c77 Compare October 1, 2019 05:16
@msftrncs
Copy link
Contributor Author

msftrncs commented Oct 1, 2019

I rebased, and have updated the commits to reflect creating the default keyword token. I have also included a potential test, after correcting an existing one that changed because of the error records no longer being the same. Please provide an initial review of this approach, and if there is any other testing that should be done.

Add `Default` token for switch statement `default` condition clause keyword,
allowing token based syntax highlighting to indicate `default` is a keyword.

Adjusted SwitchStatementRule to utilize new token.
clauseCondition is no longer produced for the `Default` clause.
Correct 'switch statement parsing' test for change in error output due to
`default` now being a keyword.
Add tests for parsing 'limited' keywords, `default`, `hidden`, `in`,
and `static` to check the token results as expected for syntax high-
lighting applications.  Validates the keywords can be used as function
names.

This is preliminary (WIP) concept.
@msftrncs msftrncs force-pushed the SwitchDefaultAsKeyword branch from 6240c77 to e7f6c17 Compare October 3, 2019 04:47
@msftrncs
Copy link
Contributor Author

msftrncs commented Oct 3, 2019

I have revised the commits again, though the bulk of changes is now in the tests. I still have the feeling the tests are still a work in progress, though they are closer to what I was first expecting.

@msftrncs msftrncs changed the title WIP: Flag default switch statement condition clause as keyword Flag default switch statement condition clause as keyword Oct 15, 2019
@msftrncs
Copy link
Contributor Author

I'm removing the WIP prefix, as I think this is ready for review. I am quite interested for feedback.

@iSazonov
Copy link
Collaborator

@daxian-dbw @JamesWTruher @adityapatwardhan Please review the PR.

@daxian-dbw daxian-dbw added this to the 7.1.0-preview.2 milestone Apr 1, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Looks like a started a review on this PR some time ago but didn't post it... Comments left unchanged

Test-ErrorStmt 'switch (1) {foo}' 'switch (1) {foo' 'foo' '1' '1' '1'
Test-ErrorStmt 'switch (1) {foo {bar}' 'switch (1) {foo {bar}' 'foo' '{bar}' 'bar' 'bar' 'bar' '1' '1' '1'
Test-ErrorStmt 'switch (1) {default {9} default{2}' 'switch (1) {default {9} default{2}' 'default' '{9}' '9' '9' '9' 'default' '{2}' '2' '2' '2' '1' '1' '1'
Test-ErrorStmt 'switch (1) {default {9} default{2}' 'switch (1) {default {9} default{2}' '{9}' '9' '9' '9' '{2}' '2' '2' '2' '1' '1' '1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like keeping 'default' in as a clause is important. We should have a test for that.

switch ('default')
{
    'default' { 'Hi' }
    default { 'Bye' }
}

is valid PowerShell (returns Hi)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the answer here is keep the test as it now is, plus add a success test to ensure that a block like the one above remains valid (and returns the correct result)

Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Ok, the code here looks good to me. Thanks for adding thorough tests!

@iSazonov iSazonov added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 3, 2020
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

@msftrncs Sorry that this PR has been overlooked for sooo long ...
The changes look good to me. I made some minor changes to keep the backward compatibility around the ErrorStatement case.

@daxian-dbw daxian-dbw merged commit 2ea18ee into PowerShell:master Jun 9, 2020
@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 9, 2020
@iSazonov
Copy link
Collaborator

@msftrncs Thanks for your contribution!

@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

@daxian-dbw daxian-dbw removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

switch statement default clause condition keyword should be flagged as keyword token

6 participants