| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Just some minor comments and questions.
Sorry, something went wrong.
There was a problem hiding this comment.
Why cannot ? and : just be added to CharTraits.ForceStartNewTokenAfterNumber? This trait is only used while the tokenizer is in expression mode.
Sorry, something went wrong.
There was a problem hiding this comment.
I might answer my own question … because it needs to be hidden behind an experimental feature flag at this time?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think the PR context explained clearly enough what this is for:
(if the following are entire command lines)
1?2:3 is a command
1-lt2?3:4 is a ternary expression.
It will be even harder for EditorSyntax (PowerShell/EditorSyntax#184) to correctly syntax scope this via TextMate. This will require an extra set of numericConstant rules to handle the 'first possible operand of an expression' vs 'a numeric value clearly in expression mode' vs 'a numeric constant in command mode'. However, it is an interesting challenge to see if I can simplify.
Sorry, something went wrong.
There was a problem hiding this comment.
Comments were added for the parameter forceEndNumberOnTernaryOperatorChars.
/// <param name="forceEndNumberOnTernaryOperatorChars">
/// In some cases, we want '?' and ':' to end a number token too, so they can be
/// treated as the ternary operator tokens.
Also, comments were added in Parser.cs to the places where the parameter endNumberOnTernaryOpChars was added.
Sorry, something went wrong.
|
Travis Plunk (@TravisEz13) Your comments have been addressed. Can you please take another look? |
Sorry, something went wrong.
There was a problem hiding this comment.
Please respond to the comment about moving parsing tests to parsing.tests.ps1
Sorry, something went wrong.
There was a problem hiding this comment.
the only real blocker for me is that I believe we should keep the parser tests together, the behavioral tests can be separated, but I would like to have all the parser tests in the same location.
Sorry, something went wrong.
|
Aditya Patwardhan (@adityapatwardhan) and James Truher (@JamesWTruher) Your comments are addressed. I rebased the branch with the master branch. The last 2 commits are changes that addressed your comments. Please take another look, thanks! |
Sorry, something went wrong.
|
James Truher (@JamesWTruher) Can you update your review? |
Sorry, something went wrong.
There was a problem hiding this comment.
thanks for making the test changes
Sorry, something went wrong.
|
It seems we should have only one CL-* label. |
Sorry, something went wrong.
|
Ilya (@iSazonov) With experimental, we should have a second, but only experimental. |
Sorry, something went wrong.
|
🎉v7.0.0-preview.4 has been released which incorporates this pull request.:tada: Handy links: |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Add support to ternary operator <condition> ? <if-true> : <if-false>. It results in a TernaryExpressionAst.
Address #3239
RFC: PowerShell/PowerShell-RFC#218
The old draft PR where a lot of discussions happened: #10161
The ternary operator behaves like the simplified if-else statement. The condition-expression will always be evaluated, and its result will be converted to boolean to determine which branch will be evaluated next:
PR Context
Ternary operator has lower precedence than binary operator, so you can write $a -eq $b ? "Hello World" : [int]::MaxValue.
In order to make the number constant expression work naturally with ternary operators, such as in return ${succeed}?0:1, characters ? and : are made start-new-token chars conditionally when scanning for numbers.
When it's known for sure that we are expecting an expression, allowing a generic token like 123? is not useful and bound to result in parsing errors.
In those cases, we will force to start a new token upon seeing characters ? and : when scanning for a number, so that that number constant expressions can work with ternary operators more intuitively.
Note that, the implicit line continuance for ? support was reverted in the latest code change based on the feedback I received within the team.
PR Checklist