Support -CustomPipeName #871
Conversation
|
lets go ahead and switch these to a normal PR for review. |
| @@ -427,20 +426,27 @@ protected void Stop() | |||
|
|
|||
| return; | |||
| } | |||
| } | |||
| else if (customPipeNameIsSet) | |||
There was a problem hiding this comment.
Do all of the clauses above return? If so, is it possible to remove this else?
There was a problem hiding this comment.
They don't all return. We have to keep this else in.
| } | ||
| else if (customPipeNameIsSet) | ||
| { | ||
| if (runspaceVersion.Version.Major < 6 && runspaceVersion.Version.Minor < 2) |
There was a problem hiding this comment.
Although this works, it strikes me as less obvious than runspaceVersion.Version <= new Version(6, 1), which in the context of remoting is worth the minor allocation overhead to improve readability
There was a problem hiding this comment.
(It's also something that could be cached statically if we really wanted to)
| $"Could not attach to process with CustomPipeName: '{attachParams.CustomPipeName}'"); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
| else |
There was a problem hiding this comment.
Might be a good opportunity to take this out?
There was a problem hiding this comment.
Given they additional check to make sure processId is an int, refactoring to remove else just doesn't seem as readable as this.
| @@ -454,6 +460,20 @@ protected void Stop() | |||
| return; | |||
| } | |||
|
|
|||
| // Clear any existing breakpoints before proceeding | |||
| await ClearSessionBreakpointsAsync().ConfigureAwait(false); | |||
There was a problem hiding this comment.
| await ClearSessionBreakpointsAsync().ConfigureAwait(false); | |
| await ClearSessionBreakpointsAsync().ConfigureAwait(continueOnCapturedContext: false); |
| int runspaceId = attachParams.RunspaceId > 0 ? attachParams.RunspaceId : 1; | ||
| _waitingForAttach = true; | ||
| Task nonAwaitedTask = | ||
| _editorSession.PowerShellContext |
There was a problem hiding this comment.
Maybe put this on the line above
TylerLeonhardt
force-pushed
the
support-custom-pipename
branch
from
be5bf58
to
fc7b4ad
Compare
| .ContinueWith(OnExecutionCompletedAsync); | ||
| await _editorSession.PowerShellContext.ExecuteScriptStringAsync( | ||
| $"Enter-PSHostProcess -CustomPipeName {attachParams.CustomPipeName}", | ||
| errorMessages); |
There was a problem hiding this comment.
Can you still get intellisense while in the attached process? I would think that this would hold up the request queue, or does Enter-PSHostProcess return in the original runspace after attaching?
There was a problem hiding this comment.
There is so little time between Enter-PSHostProcess and the Debug-Runspace call bellow that it doesn't really matter. Debug-Runspace will for sure block any intellisense call until a breakpoint is hit. Then once a breakpoint is hit, it will work again but from the context of the breakpoint.
There was a problem hiding this comment.
Does that answer your question?
FWIW, I'm not changing that behavior in this PR. In the old version it still:
awaited Enter-PSHostProcess
not awaited Debug-Runspace
There was a problem hiding this comment.
Talked about this offline. We are good.
In PowerShell 6.2 RC we will introduce
-CustomPipeName. The work is in this PR:PowerShell/PowerShell#8889
This, along with PowerShell/vscode-powershell#1775 will add support for this parameter in the debug config that is passed to
marking this as a Draft PR until RC is released.