Add -Runspace parameter to all *-PSBreakpoint cmdlets #10492
Conversation
|
@KirkMunro At first look there is a lot of style changes in the PR. Could you please move them in another PR? It would be nice for code review. |
@iSazonov I would rather not, unless I absolutely have to. This PR was originally part of #10189, which they asked to be broken up into other PRs. After a side discussion between @TylerLeonhardt, @PaulHigin and @SteveL-MSFT, they suggested this set of changes (adding the The only real "style changes" are replacement of cmdlet parameter property/field pairs with just properties, using static strings where appropriate (for parameter set names), and adding regions to make the set of breakpoint cmdlet classes consistent with one another. They were made while setting up base classes for the breakpoint cmdlets, which makes the code much easier to maintain going forward. I don't think they should really get in the way of this code review. |
|
@KirkMunro I only want to speed up a code review of the PR. It is possible for reviewer to check up to 500 lines of code at a time. The PR has over 1000 changes. Also style changes distract and slows down code review too. |
The end result of the refactoring is leaner, consistent, better defined, DRY code that is easier to work with and maintain going forward. In this case, I don't believe separating some of the changes out into a separate PR buys us anything except for more work all around. |
|
@PaulHigin Can you have a look at this? |
PaulHigin
left a comment
There was a problem hiding this comment.
I am not sure if this is the right direction to take for managing breakpoints in arbitrary runspaces. This has pretty big impact and so I feel there should be a formal design and sign-off before implementation. I'll add a committee review tag to get their opinion.
src/Microsoft.PowerShell.Commands.Utility/commands/utility/PSBreakpointUpdaterCommandBase.cs
Show resolved
Hide resolved
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
|
@adityapatwardhan and @PaulHigin: I have merged in master and resolved all conflicts. The single Codacy issue is incorrect, suggesting I should make a method static that cannot be made static. The PowerShell-CI-macos failures seem to have nothing to do with this PR. I have also manually re-reviewed the code changes to ensure that nothing unexpected came from the merge. From my perspective, this PR is good to go. |
|
@PaulHigin Please re-review the PR. @KirkMunro Thanks for the updates |
|
@KirkMunro Please open new issue in PowerShell-Docs repository. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@iSazonov: Docs issue filed, and this PR has been updated with the link at the top, so it should be good to go. |
|
@KirkMunro Please resolve the merge conflict. |
|
@adityapatwardhan: Done. |
|
@KirkMunro Thank you for your contribution and your patience! |
|
🎉 Handy links: |
PR Summary
-RunspaceparameterGet-PSBreakpoint,Set-PSBreakpoint,Enable-PSBreakpoint,Disable-PSBreakpoint, andRemove-PSBreakpoint. This parameter is added to the "Microsoft.PowerShell.Utility.PSManageBreakpointsInRunspace" experimental flag that was added in PR Add APIs for breakpoint management in runspaces and enable attach to process without BreakAll for PSES #10338.*-PSBreakpointcmdlets so that they all work consistently. This refactoring did not change the behavior of these cmdlets beyond adding support for runspace breakpoint management, with one exception:Enable-PSBreakpointso that it is more performant when passed in breakpoints via a pipeline. The previous configuration for this cmdlet would always result in the "Id" parameter set being used, even when you pipe in a breakpoint. This change does not impact the functionality of the cmdlet -- it still accepts pipeline input and command line input with named or unnamed parameters the same way. The only visible impact is that this cmdlet will prompt users for a breakpoint instead of an ID now. See Issue Parameter binding pipeline input, and how the default parameter set influences that behavior #10188 for a discussion related to this.PR Context
Now that the SDK has been enhanced to support breakpoint management in runspaces (see PR #10338), this PR builds on that work, exposing a common
-Runspaceparameter in all*-PSBreakpointcmdlets so that users can use the same cmdlets to manage breakpoints in their local session or in a remote runspace.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.