Repository navigation
List experimental feature for NewPSBreakpoint in Module manifest for Windows - #9907
TylerLeonhardt wants to merge 1 commit into
Conversation
|
Close and reopen the PR to force Windows CI build working properly. |
|
TylerLeonhardt I added some feedback to the other PR since it was merged. See here for the first of two comments: #8923 (comment). Can you review and share your thoughts on that? |
Dongbo Wang (daxian-dbw)
left a comment
There was a problem hiding this comment.
Sorry for the delay. It turns out I didn't submit my review ...
| ExperimentalFeatures = @( | ||
| @{ | ||
| Name = 'Microsoft.PowerShell.Utility.PSDebugRunspaceWithBreakpoints' | ||
| Description = "Enables the New-PSBreakpoint cmdlet and the -Breakpoint parameter on Debug-Runspace to set breakpoints in another Runspace upfront." |
There was a problem hiding this comment.
New-PSBreakpoint is an experimental feature, then it should be exposed only if the feature is turned on, right?
If that's true, then the CmdletsToExport should be conditional based on $EnabledExperimentalFeatures -contains 'Microsoft.PowerShell.Utility.PSDebugRunspaceWithBreakpoints'
There was a problem hiding this comment.
Just a side question, do we really need this to be an experimental feature? What's the thoughts on putting it behind experimental flag? (design may be change? might cause regression?)
There was a problem hiding this comment.
Dongbo Wang (@daxian-dbw): I'm glad it is listed as experimental, because there are some challenges with the design. See: #8923 (comment).
There was a problem hiding this comment.
FYI, the New-PSBreakpoint cmdlet has been removed in a PR that should be submitted today. I've also removed the experimental feature flag, because I think it is not needed anymore. We can work out details in that PR, and I'll happily add back the experimental feature flag (fully, including what was missed the first time) if necessary. This PR can probably be closed once I have that PR open, so I'll comment back here soon.
There was a problem hiding this comment.
I've also removed the experimental feature flag, because I think it is not needed anymore.
The experimental feature flag is still needed. That will be my first piece of feedback.
There was a problem hiding this comment.
But you haven't even seen the code... 😛
|
TylerLeonhardt Can you please address this comment: #9907 (comment) when you have time? Thanks |
|
Dongbo Wang (@daxian-dbw) since Kirk Munro (@KirkMunro) is close to submitting a PR that replaces this, we can just close this PR when that is out. |
|
Here is the PR that supercedes this: #10189. It uses experimental flags for cmdlet parameter additions. If an RFC is necessary to accompany that PR, let me know. |
|
Thanks Kirk! |
PR Summary
In #8923 I forgot to add this to the module manifest for Utility for Windows. This PR adds it.
I should probably speak to Dongbo Wang (@daxian-dbw) about how we can verify experimental features exist before proceeding (which would have caught this issue)... but I'm going to call that out of scope unless Dongbo Wang (@daxian-dbw) thinks it's trivial to add.
PR Context
Without this, the Experimental Feature does not show up.
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.