-
-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for terminals that redirect input such as MINGW64 #3
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
base: main
Are you sure you want to change the base?
Add support for terminals that redirect input such as MINGW64 #3
Conversation
N.B.: @patriksvensson This is only updating the |
@@ -81,41 +86,38 @@ public sealed class Settings : CommandSettings | |||
} | |||
|
|||
private static SnapshotAction ShowPrompt() |
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.
There is no longer a loop until the user submits a valid command. We need to add that.
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.
@patriksvensson The loop is there, it's just been moved up in the call stack. Typing anything other than a valid command will re-render the diff and ask for the prompt again.
@@ -81,41 +86,38 @@ public sealed class Settings : CommandSettings | |||
} | |||
|
|||
private static SnapshotAction ShowPrompt() | |||
{ |
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.
I would also want this functionality to be a fallback method instead of replacing what we currently have. We can check if the input has been redirected, and fallback to this new functionality.
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.
@patriksvensson Sure, this is definitely possible to do - technically speaking that is -, but are you sure this is a good idea? 🤔 There are many consoles that redirect the input, including the terminal in Visual Studio Code... So a user could easily have different UX using this tool on the same machine depending on the terminal they happen to be using each time... Seems a little odd if you ask me.
Also, using ReadLine
for prompting for an answer is the approach we see on popular command-line tools like Git, Homebrew, npm, AWS Cli, Azure Cli, and many others... Can you think of any modern/popular command-line tools that use ReadKey
? I suspect there aren't many because of this exact limitation on input redirection...
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.
Yes, "Insta" which I based this tool on, for one.
I don't want to have to press enter for each file when I review them, so if stdin isn't redirected (the usual case when running an interactive app), I want to grab input without having to press enter if possible.
Given that it's not possible to use
Console.ReadKey
when the input is redirected, I'm suggesting an approach usingConsole.ReadLine
to display a prompt (with a visible cursor).In other words, the user now has to hit
<enter>
after typing one of the letters (a
,r
, ors
).As a bonus, the full words
accept
,reject
, andskip
are also valid.Closes #1