-
Notifications
You must be signed in to change notification settings - Fork 223
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
PSReadLine integration (WIP) #671
PSReadLine integration (WIP) #671
Conversation
Adds classes that manage the state of the prompt, nested contexts, and multiple ReadLine implementations of varying complexity.
Change ReadLine method to call out to PowerShellContext. This lets the PowerShellContext determine which ReadLine implementation to use based on available modules. Also includes some changes to the System.Console proxy classes to account for PSReadLine.
Refactor PowerShellContext to have a more robust system for tracking the context in which commands are invoked. This is a significant change in that all interactions with the runspace must be done through methods in PowerShellContext. These changes also greatly increase stability.
All interactions with the runspace must be done through PowerShellContext now that nested PowerShell instances are encountered frequently. Also fix a bunch of race conditions that were made more obvious with the changes.
$ExecutionContext, | ||
$args[0])"; | ||
|
||
// private const string ReadLineScript = @" |
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.
remove or re-consider commented out code
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.
Nice catch! Must have missed that one on my sweep :)
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.
No worries, as you said, the full review will only come after the conflict resolution and especial when it is not WIP any more. Will this bring history functionality (PowerShell/vscode-powershell#550) for 'free' out of the box?
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.
It does indeed :)
Mostly anyway. The commands we invoke in the background still show up in Get-History
and still increment $MyInvocation.HistoryId
which is annoying. But navigating history with all the PSRL hotkeys like CTRL + R/up/down/etc work as expected in the debugger as well :)
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.
Caught some more mistakes, reminding myself to fix.
@@ -779,8 +804,7 @@ protected void Stop() | |||
i)); | |||
} | |||
|
|||
await requestContext.SendResult( | |||
new StackTraceResponseBody | |||
await requestContext.SendResult( new StackTraceResponseBody |
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.
Mistake, must have done a vim line join and didn't notice.
@@ -1463,6 +1463,15 @@ private static FileChange GetFileChangeDetails(Range changeRange, string insertS | |||
catch (TaskCanceledException) | |||
{ | |||
// If the task is cancelled, exit directly | |||
foreach (var script in filesToAnalyze) |
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.
Comment needs to be updated.
VariableContainerDetails.AutoVariablesName); | ||
// If access to stackFrameDetails isn't controlled there is a race condition where | ||
// the array isn't finished populating before | ||
// await this.stackFramesHandle.WaitAsync(); |
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.
More commented code that should be removed or uncommented.
} | ||
} | ||
// } |
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.
More commented code that should be removed or uncommented.
@@ -52,6 +52,21 @@ public class CommandHelpers | |||
return null; | |||
} | |||
|
|||
// Keeping this commented out for now. It would be faster, but it doesn't automatically | |||
// import modules. This may actually be preferred, but it's a big change that needs to |
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.
This comment should be removed and replaced with an issue.
|
||
// Ensure the result is set in another thread otherwise the caller | ||
// may take over the pipeline thread. | ||
System.Threading.Tasks.Task.Run(() => SetResult(true)); |
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.
Replace fully qualified type name with using statement.
// If a ReadLine pipeline is running in the debugger then we'll hang here | ||
// if we don't cancel it. Typically we can rely on OnExecutionStatusChanged but | ||
// the pipeline request won't even start without clearing the current task. | ||
// await this.PromptContext.AbortReadLineAsync(); |
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.
Remove commented code.
{ | ||
// Set the result so that the execution thread resumes. | ||
// The execution thread will clean up the task. | ||
|
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.
Extra new line.
null, | ||
false, | ||
false, | ||
false); |
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.
bool
parameters should be named.
@@ -1239,7 +1634,7 @@ private IEnumerable<TResult> ExecuteCommandInDebugger<TResult>(PSCommand psComma | |||
if (debuggerResumeAction.HasValue) | |||
{ | |||
// Resume the debugger with the specificed action | |||
this.ResumeDebugger(debuggerResumeAction.Value); | |||
this.ResumeDebugger(debuggerResumeAction.Value, false); |
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.
bool
parameter should be named.
Had some issues with merge conflict resolution, continuing with PR #672 |
Need to resolve a significant amount of merge conflicts, but I wanted to get this up so folks know it's close. Tests probably need some (or a lot of) fixing as well.
The appveyor build is unlikely to work until I resolve the merge conflicts due to changes to the startup script. Also requires the PSReadLine build from this PR PowerShell/PSReadLine#626
Highlights:
$Host.EnterNestedPrompt()
supportI'd suggest holding off on formally reviewing until the merge conflicts are resolved. After that's done I'll also update this description with significantly more detail about the changes.
Anyone curious about the changes required for PSRL should check out specifically the classes PowerShellContext, InvocationEventQueue, PromptNest, and PSReadLinePromptContext.