-
Notifications
You must be signed in to change notification settings - Fork 237
Track tempIntegratedConsole launch param, do not exit when session ends #625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -303,6 +303,12 @@ private void OnDebugServiceClientConnect(object sender, TcpSocketServerChannel s | |||
|
|||
this.debugServiceListener.Start(); | |||
} | |||
else if (this.debugAdapter.IsUsingTempIntegratedConsole) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this else if prevents the host from exiting... What's the host in this case?
More simply, what is the user experience change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temp debug console does not immediately exit after the script finishes execution or the debug session is stopped. This allows the user to scroll through the window to inspect the script output.
The only downside I've observed at this point is that every temp debug session log file ends with:
2/19/2018 8:48:16 PM [ERROR] - Method "ListenForMessages" at line 344 of C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices\src\PowerShellEditorServices.Protocol\MessageProtocol\ProtocolEndpoint.cs
Stream terminated unexpectedly, ending MessageDispatcher loop
Exception: IOException
Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host.
This is because VSCode does terminate the process when it starts the next debug session. All things considered, I think this is a fine trade-off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as soon as AppVeyor passes. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks Keith!
This does not leave a working prompt. I think that is OK for now. This is still better than the current situation.