Flag default switch statement condition clause as keyword#10487
Flag default switch statement condition clause as keyword#10487daxian-dbw merged 4 commits intoPowerShell:masterfrom
default switch statement condition clause as keyword#10487Conversation
|
I may need guidance on tests, I have not yet ventured in to any of the tests so far. |
|
I do not find any tests for keywords. |
|
As for tests, it looks like, adding to |
19bc872 to
6240c77
Compare
|
I rebased, and have updated the commits to reflect creating the |
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.
6240c77 to
e7f6c17
Compare
|
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. |
default switch statement condition clause as keyworddefault switch statement condition clause as keyword
|
I'm removing the WIP prefix, as I think this is ready for review. I am quite interested for feedback. |
|
@daxian-dbw @JamesWTruher @adityapatwardhan Please review the PR. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
rjmholt
left a comment
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
rjmholt
left a comment
There was a problem hiding this comment.
Ok, the code here looks good to me. Thanks for adding thorough tests!
daxian-dbw
left a comment
There was a problem hiding this comment.
@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.
|
@msftrncs Thanks for your contribution! |
|
🎉 Handy links: |
PR Summary
Flag the
switchstatement condition clausedefaulttoken as a keywordtoken to facilitate (PSReadLine) highlighting it as a keyword. Closes #10470.
This change is to treat the
defaultclause keyword as a full keyword token, and as such token based highlighters will see it as a keyword.PR Context
Flagging the
defaultcondition clause token of theswitchstatement as a keyword will facilitate highlighting of the keyword to better contrast against other bareword string literal conditions.A simple (partial) example:
Highlighting of the
defaultkeyword in the above example makes it easier to distinguish the special condition clause from the other bareword conditions.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.