Make approved features non-experimental #11303

SteveL-MSFT · 2019-12-09T21:05:33Z

PR Summary

As discussed in @PowerShell/powershell-committee, these features will be out of Experimental status:

PSCoalescingOperators
PSErrorView
PSForEachObjectParallel
PSPipelineChainOperators
PSTernaryOperator
PSUpdatesNotification
PSWindowsPowerShellCompatibility
Microsoft.PowerShell.Utility.PSGetError

Code marking them as Experimental has been removed.

PR Context

Fix #11302
Fix #11081

PR Checklist

anmenaga

PSWindowsPowerShellCompatibility has to remain experimental. Primary reasons:

not enough flight time yet. First time it appeared only few weeks ago in Preview 6. On high level this feature changes module load behavior for some modules so it has capability to change/mess up existing user scripts/scenarios that were running fine before it. We've seen this happened with ScheduledJobs and CertificateProvider.
This needs more flight time.
As continuation from #1 if something goes wrong because of this feature - currently the only mechanism to turn it off is ExperimentalFeature switch.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Get-Error.cs

SteveL-MSFT · 2019-12-09T21:43:17Z

CodeFactor issues are unrelated to my specific changes

rjmholt · 2019-12-09T21:43:59Z

src/System.Management.Automation/engine/parser/Parser.cs

            // G      binary-expression   '?'   new-lines:opt   ternary-expression   new-lines:opt   ':'   new-lines:opt   ternary-expression

            // TODO: remove this if-block when making 'ternary operator' an official feature.
-            if (!ExperimentalFeature.IsEnabled("PSTernaryOperator"))


Counterpoint here: if we want a backward compatible parser/tokenizer, we should keep some kind of static flags in so that we could eventually create an implementation where you can build instances of parsers targeting different versions or feature sets.

Just preserving some element of the code in here will preserve enough information for us to unpick all this later.

Would a comment be sufficient?

// Added in PS7.0

Comments won't really describe the way the logic branches, but if we really want to avoid different logic (or changing the conditions to const bools) then we can muddle through with something designed to look and be machine parseable for a regex like // ps7-expfeat:PSTernaryOperator or something

If we really want to pursue the build instances of parsers targeting different versions or feature sets goal, then we will have to guard the to-be-removed code in a compiler definition, like #if PSV6 ... #endif. I can see why we want this but it won't really help unless we test those guarded code on regular basis, and eventually this will make the code difficult to maintain.

Perhaps we should revisit this later.

SteveL-MSFT · 2019-12-09T22:16:24Z

@anmenaga if this feature becomes blocking/impactful negatively for customers, we would be ok taking a change to add a setting to powershell.config.json to disable this specific feature.

src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs

src/System.Management.Automation/engine/parser/Parser.cs

PaulHigin

LGTM

adityapatwardhan · 2019-12-10T17:52:31Z

@SteveL-MSFT Please resolve merge conflicts

…table

src/System.Management.Automation/engine/parser/Parser.cs

daxian-dbw · 2019-12-10T18:13:54Z

src/System.Management.Automation/engine/parser/Parser.cs

            // G      binary-expression   '?'   new-lines:opt   ternary-expression   new-lines:opt   ':'   new-lines:opt   ternary-expression

            // TODO: remove this if-block when making 'ternary operator' an official feature.
-            if (!ExperimentalFeature.IsEnabled("PSTernaryOperator"))


If we really want to pursue the build instances of parsers targeting different versions or feature sets goal, then we will have to guard the to-be-removed code in a compiler definition, like #if PSV6 ... #endif. I can see why we want this but it won't really help unless we test those guarded code on regular basis, and eventually this will make the code difficult to maintain.

src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs

daxian-dbw · 2019-12-10T18:24:44Z

I only reviewed the code changes related to ternary operator and update notification. Feature owners please review changes related to your features and sign off.

daxian-dbw · 2019-12-10T18:29:10Z

if this feature becomes blocking/impactful negatively for customers, we would be ok taking a change to add a setting to powershell.config.json to disable this specific feature.

@SteveL-MSFT when are we going to know it becomes blocking? Do you mean after 7.0.0 GA, we can make changes in the servicing branch of 7.0.0 to add back the experimental feature declaration and checks for the win-module-compat work?

SteveL-MSFT · 2019-12-10T18:45:22Z

@daxian-dbw I opened a new issue #11309 which we can accept for GA

adityapatwardhan · 2019-12-10T18:53:53Z

@SteveL-MSFT Please fix build failures.

adityapatwardhan · 2019-12-10T19:46:54Z

@daxian-dbw Please re-review.

daxian-dbw · 2019-12-10T22:42:12Z

test/tools/TestMetadata.json

@@ -1,7 +1,5 @@
 {
  "ExperimentalFeatures": {
-    "ExpTest.FeatureOne": [ "test/powershell/engine/ExperimentalFeature/ExperimentalFeature.Basic.Tests.ps1" ],


Ideally, you should add entries for those features that are still kept as experimental, because otherwise, their tests will not be evaluated during the GA release build.
But their tests will still be run in our daily build (since all experimental features are enabled in daily builds), so that can be deferred to another PR.

# Conflicts: # src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs

iSazonov · 2019-12-11T04:42:24Z

Make sense to add CL- label?

ghost · 2019-12-16T21:38:00Z

🎉v7.0.0-rc.1 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

SteveL-MSFT requested review from PaulHigin, adityapatwardhan, anmenaga, daxian-dbw and rjmholt December 9, 2019 21:05

SteveL-MSFT assigned adityapatwardhan Dec 9, 2019

SteveL-MSFT added the MustHave label Dec 9, 2019

SteveL-MSFT added this to the rc.1-consider milestone Dec 9, 2019

anmenaga suggested changes Dec 9, 2019

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 9, 2019

Make approved features non-experimental

24821f2

SteveL-MSFT force-pushed the expr-stable branch from 8829031 to 24821f2 Compare December 9, 2019 21:11

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 9, 2019

PoshChan reviewed Dec 9, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Get-Error.cs Show resolved Hide resolved

PoshChan reviewed Dec 9, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Get-Error.cs Show resolved Hide resolved

PoshChan reviewed Dec 9, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Get-Error.cs Show resolved Hide resolved

fix test

ecd491b

rjmholt reviewed Dec 9, 2019

View reviewed changes

adityapatwardhan requested changes Dec 9, 2019

View reviewed changes

src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs Show resolved Hide resolved

src/System.Management.Automation/engine/parser/Parser.cs Outdated Show resolved Hide resolved

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 9, 2019

PaulHigin approved these changes Dec 9, 2019

View reviewed changes

anmenaga approved these changes Dec 9, 2019

View reviewed changes

address Aditya's comments

008581f

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Dec 9, 2019

Merge branch 'master' of github.com:PowerShell/PowerShell into expr-s…

376a3c8

…table

daxian-dbw reviewed Dec 10, 2019

View reviewed changes

address Dongbo's feedback

916df99

SteveL-MSFT requested a review from TravisEz13 as a code owner December 10, 2019 19:27

adityapatwardhan approved these changes Dec 10, 2019

View reviewed changes

daxian-dbw reviewed Dec 10, 2019

View reviewed changes

daxian-dbw approved these changes Dec 10, 2019

View reviewed changes

SteveL-MSFT modified the milestones: rc.1-consider, rc.1-approved Dec 10, 2019

adityapatwardhan merged commit ed1f6e3 into PowerShell:master Dec 10, 2019

TravisEz13 pushed a commit that referenced this pull request Dec 11, 2019

Make approved features non-experimental (#11303)

fee57e1

# Conflicts: # src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs

TravisEz13 modified the milestones: rc.1-approved, 7.0.0-rc.1 Dec 11, 2019

daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 13, 2019

This was referenced Dec 16, 2019

Certain errors are not shown when all experimental features are disabled #11081

Closed

Decide on which experimental features will remain in experimental in 7.0 #11302

Closed

SteveL-MSFT deleted the expr-stable branch June 6, 2020 02:30

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Update list of PowerShell team members in release tools #18266

Merged

22 tasks

Make approved features non-experimental #11303

Make approved features non-experimental #11303

Uh oh!

Conversation

SteveL-MSFT commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

anmenaga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SteveL-MSFT commented Dec 9, 2019

Uh oh!

rjmholt Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

rjmholt Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

rjmholt Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Dec 9, 2019

Uh oh!

Uh oh!

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Dec 10, 2019

Uh oh!

Uh oh!

daxian-dbw Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daxian-dbw commented Dec 10, 2019

Uh oh!

daxian-dbw commented Dec 10, 2019

Uh oh!

SteveL-MSFT commented Dec 10, 2019

Uh oh!

adityapatwardhan commented Dec 10, 2019

Uh oh!

adityapatwardhan commented Dec 10, 2019

Uh oh!

daxian-dbw Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Dec 11, 2019

Uh oh!

ghost commented Dec 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

SteveL-MSFT commented Dec 9, 2019 •

edited

Loading

daxian-dbw Dec 10, 2019 •

edited

Loading