-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add support for Ctrl-C to interrupt commands, and add multi-line capa… #613
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
…bility for script debugger input
Console.Write("[DBG] PS >> "); | ||
if (incompleteLine) | ||
{ | ||
Console.Write(">> "); |
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.
Shouldn't this be consoleReadLine.Write
? So it goes through our Readline
class instead of straight through to System.Console
.
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.
Is that what we should use? It's never occurred to me that we should write through ConsoleReadLine. It is "Read"-Line after all :)
ConsoleReadLine currently has no "Write" methods at all, and we use Console.WriteLine() everywhere.
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.
Sorry, I mean this.myHost.UI.Write
.
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.
We use Console.Write or Console.Out.Write all over the place. If these calls, along with Cursor, foreground/background color, etc., all should be moved to myHost.UI, then please open a separate bug.
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.
Done, #616.
Your notes in the PR description are what should be the body of the commit message. Keep in mind that PRs are not permanent (they exist only on GitHub); only the Git commit messages are actually tied to the project. Be as descriptive as you can with your commit messages. |
Just reiterating information from phone conversations; we have a couple things going on here:
|
I'm not too concerned about responsiveness. I did a quick web search, and the fastest one could type is about 12 strokes a second, or about 83 milliseconds between strokes. If I change sleep to 80 milliseconds, it should be plenty to keep up with fastest typist. As for second item, I assume a fix is forth coming. Any estimate on when they will fix that? I currently don't see this problem. Have they released the broken patch, or will wait for the new fix before releasing everything? |
Since the resolution of this PR will not be immediate, I will take changes associated with multi-line out of this PR, and create a separate PR for it. |
You don't see the problem because the pinned packages are before any of the changes. The fix is already up in dotnet/corefx#6619; I didn't catch it because we weren't using Thanks @palladia. |
New push that includes most changes per Andy comments. Ctrl-C works great, and with 80 msec sleep, there's no apparent lag. Will re-test after CoreFx #6619 is pulled into our build. |
I'd prefer to wait until after we've merged in the new packages, so we can test before merging. |
No problem. |
Tested on latest .NET updates... this performs very oddly. |
|
Closing in favor of #717, right? |
This update fixes two things:
This change is