-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Flag default switch statement condition clause as keyword
#10487
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
Conversation
|
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.
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' |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
rjmholt
left a comment
There was a problem hiding this 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!
daxian-dbw
left a comment
There was a problem hiding this 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.
|
@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.