FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support ternary operator in PowerShell language by daxian-dbw · Pull Request #10367 · PowerShell/PowerShell · GitHub

Support ternary operator in PowerShell language - #10367

Merged
Aditya Patwardhan (adityapatwardhan) merged 20 commits into
PowerShell:masterfrom
daxian-dbw:ternary
Sep 4, 2019
Merged

Support ternary operator in PowerShell language#10367
Aditya Patwardhan (adityapatwardhan) merged 20 commits into
PowerShell:masterfrom
daxian-dbw:ternary

Conversation

Dongbo Wang (daxian-dbw) commented Aug 15, 2019
edited
Loading

Copy link
Copy Markdown
Member

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:

  • if-true-expression will execute if the condition's result is evaluated as true
  • if-false-expression will execute if the condition's result is evaluated as false

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just some minor comments and questions.

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 17, 2019
Ilya (iSazonov) added this to the 7.0.0-preview.3 milestone Aug 17, 2019
ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 19, 2019
Travis Plunk (TravisEz13) added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Aug 19, 2019
ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 19, 2019

Carl Morris (msftrncs) Aug 20, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why cannot ? and : just be added to CharTraits.ForceStartNewTokenAfterNumber? This trait is only used while the tokenizer is in expression mode.

Carl Morris (msftrncs) Aug 20, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I might answer my own question … because it needs to be hidden behind an experimental feature flag at this time?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

Travis Plunk (@TravisEz13) Your comments have been addressed. Can you please take another look?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please respond to the comment about moving parsing tests to parsing.tests.ps1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 27, 2019

Copy link
Copy Markdown
Member Author

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!

Copy link
Copy Markdown
Member

James Truher (@JamesWTruher) Can you update your review?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thanks for making the test changes

Aditya Patwardhan (adityapatwardhan) added the CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log label Sep 4, 2019
Aditya Patwardhan (adityapatwardhan) merged commit 5069c7d into PowerShell:master Sep 4, 2019
Ilya (iSazonov) removed the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Sep 5, 2019

Copy link
Copy Markdown
Collaborator

It seems we should have only one CL-* label.

Travis Plunk (TravisEz13) added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Sep 5, 2019

Copy link
Copy Markdown
Member

Ilya (@iSazonov) With experimental, we should have a second, but only experimental.

Travis Plunk (TravisEz13) pushed a commit to TravisEz13/PowerShell that referenced this pull request Sep 14, 2019
Andrew (anmenaga) removed the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Sep 17, 2019

Copy link
Copy Markdown

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

Handy links:

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants


Back | FazBrowse Home | New Git URL