Console.ReadKey() implementation for SystemConsole

[thoemmi]

Generally I like the analyzer's warnings about usage of System.Console. However, in case of System.Console.ReadKey() there's no corresponding method in IConsole.

Either it may be added to IConsole, or the analyzer should stop worrying about ReadKey().

[Tyrrrz]

It should be probably added, but need to check how ReadKey() works under the hood. Calling any Console methods is not safe because the IConsole instance might not be bound to the real console. You can ignore the warning for now though as there are no alternatives.

[thoemmi]

Sounds reasonable. However, I have enabled TreatWarningsAsErrors, so I had to add #pragma warning disable CliFx0100 in my code.

[ron-myers]

I would be happy to help implement this if you have time to discuss your thoughts on the target approach @Tyrrrz

My Twitter DMs are open.

[Tyrrrz]

@ron-myers Do you already have a plan? We can discuss it here, it's easier to track this way.

[ron-myers]

@Tyrrrz - not yet.

My desire is driven more by need than a plan.

I am still learning my way around the codebase and see this as a large enough change to discuss before submitting a PR.

Likely would start by adding ReadKey/ReadLine/Read to the IConsole interface and evaluating what is needed, likely need some research to land on a decent first implementation.

[Tyrrrz]

ReadLine and Read already exist, it's only ReadKey that's missing. Need to figure out how it works under the hood, i.e. how it infers a key from stdin.

[alexrosenfeld10]

So, I looked at this a bit. It's mentioned that ReadLine and Read already exist on the IConsole interface, but I don't see that to be the case. Perhaps that comment was made prior to some refactor.

In any case, they do appear on the StreamReader (on IConsole.Input here). However... the ReadKey method is directly on the System.Console, and nowhere else. This is simple, but would it be acceptable to simply add methods to the IConsole that wraps the System.Console.ReadKey method? I would imagine not for unit testing reasons, but functionally it seems OK to me.

        /// <summary>
        /// Calls the underlying System.Console.ReadKey method.
        /// </summary>
        /// <a href="https://docs.microsoft.com/en-us/dotnet/api/system.console.readkey">See docs</a>
        /// <param name="intercept"></param>
        /// <returns></returns>
        public static ConsoleKeyInfo ReadKey(bool intercept = false) => System.Console.ReadKey(intercept);

[alexrosenfeld10]

Tangentially related, I'm curious is there some reason for keeping the Console around in the ConsoleReader? It seems unused to me.

[Tyrrrz]

Tangentially related, I'm curious is there some reason for keeping the Console around in the ConsoleReader? It seems unused to me.

This is so that you can write extension methods on the writer/reader and have access to the console to, for example, change color. This is also used internally in ConsoleFormatter too: https://github.com/Tyrrrz/CliFx/blob/51cca36d2a333213c78358eac7e6398ce7e02170/CliFx/Formatting/ConsoleFormatter.cs#L30-L34

[Tyrrrz]

So, I looked at this a bit. It's mentioned that ReadLine and Read already exist on the IConsole interface, but I don't see that to be the case. Perhaps that comment was made prior to some refactor.

In any case, they do appear on the StreamReader (on IConsole.Input here). However... the ReadKey method is directly on the System.Console, and nowhere else. This is simple, but would it be acceptable to simply add methods to the IConsole that wraps the System.Console.ReadKey method? I would imagine not for unit testing reasons, but functionally it seems OK to me.

        /// <summary>
        /// Calls the underlying System.Console.ReadKey method.
        /// </summary>
        /// <a href="https://docs.microsoft.com/en-us/dotnet/api/system.console.readkey">See docs</a>
        /// <param name="intercept"></param>
        /// <returns></returns>
        public static ConsoleKeyInfo ReadKey(bool intercept = false) => System.Console.ReadKey(intercept);

They were never on the console, always on the reader. We are just lacking a ReadKey(...) implementation as well. I was curious if the underlying implementation of System.Console.ReadKey(...) uses standard input stream or not. If it did, we could then implement that method on top of ConsoleReader. That would also make it more useful in testing because then we could actually do something instead of just no-op.

But if there is no other option, then adding a ReadKey(...) method directly on IConsole and have it no-op in console seems like the only way.

[alexrosenfeld10]

Tangentially related, I'm curious is there some reason for keeping the Console around in the ConsoleReader? It seems unused to me.

This is so that you can write extension methods on the writer/reader and have access to the console to, for example, change color. This is also used internally in ConsoleFormatter too:

https://github.com/Tyrrrz/CliFx/blob/51cca36d2a333213c78358eac7e6398ce7e02170/CliFx/Formatting/ConsoleFormatter.cs#L30-L34

Gotcha. I saw it used in the writer like that, I think it's unused in the reader in the lib but perhaps someone out there uses it. No harm in keeping :)

[alexrosenfeld10]

So, I looked at this a bit. It's mentioned that ReadLine and Read already exist on the IConsole interface, but I don't see that to be the case. Perhaps that comment was made prior to some refactor. In any case, they do appear on the StreamReader (on IConsole.Input here). However... the ReadKey method is directly on the System.Console, and nowhere else. This is simple, but would it be acceptable to simply add methods to the IConsole that wraps the System.Console.ReadKey method? I would imagine not for unit testing reasons, but functionally it seems OK to me.

        /// <summary>
        /// Calls the underlying System.Console.ReadKey method.
        /// </summary>
        /// <a href="https://docs.microsoft.com/en-us/dotnet/api/system.console.readkey">See docs</a>
        /// <param name="intercept"></param>
        /// <returns></returns>
        public static ConsoleKeyInfo ReadKey(bool intercept = false) => System.Console.ReadKey(intercept);

They were never on the console, always on the reader. We are just lacking a ReadKey(...) implementation as well. I was curious if the underlying implementation of System.Console.ReadKey(...) uses standard input stream or not. If it did, we could then implement that method on top of ConsoleReader. That would also make it more useful in testing because then we could actually do something instead of just no-op.

But if there is no other option, then adding a ReadKey(...) method directly on IConsole and have it no-op in console seems like the only way.

Cool, I will take a look further when time allows. Thanks for the info

[alexrosenfeld10]

Historically I'm not a huge .NET runtime person, I've spent most of my time in layers above this part of the stack... did some digging, here are the "connect the dots" if you will:

Seems like the underlying implementation of the Console.ReadKey just wraps the ConsolePal, which seems to have a different implementation on each platform.

Here's the unix example. The ConsolePal ultimately uses an implementation of the TextReader, the StdInReader to read the key. It has a bit of business logic thereafter to decide whether or not to write the key to the console.

On windows, it doesn't use this TextReader impl setup. It has the business logic more directly in the ConsolePal.ReadKey method, using an Interop.Kernel32.ReadConsoleInput method call, and a simple if statement to prevent writing characters to the console:

https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/libraries/System.Console/src/System/ConsolePal.Windows.cs#L394-L395

Given my background... if we're doing anything fancy here, it's probably above my head. I'd say adding our own ReadKey that just wraps the System.Console.ReadKey() is probably the way to go here.

[Tyrrrz]

I'd say adding our own ReadKey that just wraps the System.Console.ReadKey() is probably the way to go here.

Fair enough, let's do that.