Async overloads for AnsiConsole Prompt/Ask/Confirm. #1194
Conversation
|
@patriksvensson can you take a look? |
|
@patriksvensson ptal. |
|
@tmds Sorry for taking time with this; currently a bit behind on the backlog. Will try to take a look at this tonight. |
|
This is very useful. Would love to see it merged. |
FrankRay78
changed the title
FrankRay78
changed the title
|
I'll take a look at this @tmds |
|
Dear @spectreconsole/maintainers, what's our policy on unit test coverage when adding Async overloads to existing functionality. The code in this PR is clear, well structured, and high-quality. However, it doesn't contain any unit tests. We have extensive unit test coverage of prompts here: test/Spectre.Console.Tests/Unit/Prompts, however they exercise the current (non-async) calls. I don't think it makes sense to duplicate the 100-something existing unit tests for these async extensions, however equally I don't want to merge this until I've checked it's ok 'as-is' without tests. What are your thoughts? |
@mbursill Do you have a code example of how you'd like to use the async overloads, out of interest? |
|
@tmds - what is the usage for an async prompt? I need some help understanding why a synchronous user prompt should be extended to be async. I'm not saying there isn't a need, but I need a working code snippet illustrating the motivation behind this PR please. |
The |
|
@tmds ok thanks for replying after so much time. Apologies, the widgets aren't my usual area of expertise and the other maintainers are quite busy to ask. I noticed your PR which looked like a good quality contribution and taken it on myself to review/merge. I think I understand the usage now. I do plan to get this one over the line. |
|
No worries, @FrankRay78. Thanks for picking this up! |
|
@FrankRay78 Unit tests are preferred so we'll know if we introduce regressions. But it's your call. |
The current prompt APIs suffer from several problems.
First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues.
Here's are the problematic implementation (before this commit fixes it):
```csharp
public ConsoleKeyInfo? ReadKey(bool intercept)
{
return System.Console.ReadKey(intercept);
}
public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken)
{
while (true)
{
if (cancellationToken.IsCancellationRequested)
{
return null;
}
if (System.Console.KeyAvailable)
{
break;
}
await Task.Delay(5, cancellationToken).ConfigureAwait(false);
}
return ReadKey(intercept);
}
```
* The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`.
* The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged.
* The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop.
The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods.
This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs.
Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`.
I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).
The current prompt APIs suffer from several problems.
First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues.
Here's are the problematic implementation (before this commit fixes it):
```csharp
public ConsoleKeyInfo? ReadKey(bool intercept)
{
return System.Console.ReadKey(intercept);
}
public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken)
{
while (true)
{
if (cancellationToken.IsCancellationRequested)
{
return null;
}
if (System.Console.KeyAvailable)
{
break;
}
await Task.Delay(5, cancellationToken).ConfigureAwait(false);
}
return ReadKey(intercept);
}
```
* The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`.
* The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged.
* The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop.
The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods.
This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs.
Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`.
I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).
The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.
The current prompt APIs suffer from several problems.
First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues.
Here's are the problematic implementation (before this commit fixes it):
```csharp
public ConsoleKeyInfo? ReadKey(bool intercept)
{
return System.Console.ReadKey(intercept);
}
public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken)
{
while (true)
{
if (cancellationToken.IsCancellationRequested)
{
return null;
}
if (System.Console.KeyAvailable)
{
break;
}
await Task.Delay(5, cancellationToken).ConfigureAwait(false);
}
return ReadKey(intercept);
}
```
* The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`.
* The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged.
* The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop.
The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods.
This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs.
Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`.
I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).
The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.
The current prompt APIs suffer from several problems.
First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues.
Here's are the problematic implementation (before this commit fixes it):
```csharp
public ConsoleKeyInfo? ReadKey(bool intercept)
{
return System.Console.ReadKey(intercept);
}
public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken)
{
while (true)
{
if (cancellationToken.IsCancellationRequested)
{
return null;
}
if (System.Console.KeyAvailable)
{
break;
}
await Task.Delay(5, cancellationToken).ConfigureAwait(false);
}
return ReadKey(intercept);
}
```
* The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`.
* The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged.
* The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop.
The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods.
This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs.
Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`.
I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).
The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.
The current prompt APIs suffer from several problems.
First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues.
Here's are the problematic implementation (before this commit fixes it):
```csharp
public ConsoleKeyInfo? ReadKey(bool intercept)
{
return System.Console.ReadKey(intercept);
}
public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken)
{
while (true)
{
if (cancellationToken.IsCancellationRequested)
{
return null;
}
if (System.Console.KeyAvailable)
{
break;
}
await Task.Delay(5, cancellationToken).ConfigureAwait(false);
}
return ReadKey(intercept);
}
```
* The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`.
* The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged.
* The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop.
The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods.
This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs.
Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`.
I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).
The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.
|
We've been discussing with @FrankRay78 and I've been experimenting with an alternative approach to this issue. I think the async APIs actually don't make sense but the sync APIs need cancellation support. I've been working on an obsolete-async-prompt-apis branch where I have added cancellation support for all the prompt APIs. @tmds Would you mind checking out this branch, build from source and see if this approach solves your problem? If you need help building from source I can assist. |
The current prompt APIs suffer from several problems.
First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues.
Here's are the problematic implementation (before this commit fixes it):
```csharp
public ConsoleKeyInfo? ReadKey(bool intercept)
{
return System.Console.ReadKey(intercept);
}
public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken)
{
while (true)
{
if (cancellationToken.IsCancellationRequested)
{
return null;
}
if (System.Console.KeyAvailable)
{
break;
}
await Task.Delay(5, cancellationToken).ConfigureAwait(false);
}
return ReadKey(intercept);
}
```
* The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`.
* The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged.
* The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop.
The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods.
This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs.
Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`.
I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).
The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.
Can you explain why? Because the |
You're right, I'll scrap my branch and start over with only adding cancellation support to the synchronous APIs (which is still needed) but without touching the asynchronous APIs which are fine.
Note The tests that matter for this pull request are in What about modifying the existing tests like this, from fact to theory with both sync and async paths? I think it's still pretty readable. Before: [Fact]
public void Should_Return_Correct_DateTime_When_Asked_US_Culture()
{
// Given
var console = new TestConsole().EmitAnsiSequences();
console.Input.PushTextWithEnter("2/1/1998");
// When
var dateTime = console.Ask<DateTime>(string.Empty, CultureInfo.GetCultureInfo("en-US"));
// Then
dateTime.ShouldBe(new DateTime(1998, 2, 1));
}After: [Theory]
[InlineData(true)]
[InlineData(false)]
public async Task Should_Return_Correct_DateTime_When_Asked_US_Culture(bool async)
{
// Given
var console = new TestConsole().EmitAnsiSequences();
console.Input.PushTextWithEnter("2/1/1998");
// When
DateTime dateTime;
if (async)
dateTime = await console.AskAsync<DateTime>(string.Empty, CultureInfo.GetCultureInfo("en-US"));
else
dateTime = console.Ask<DateTime>(string.Empty, CultureInfo.GetCultureInfo("en-US"));
// Then
dateTime.ShouldBe(new DateTime(1998, 2, 1));
} |
|
FYI. I've removed myself as the reviewer/assignee of this PR, @0xced, given it's now part of your bigger batch of prompt improvements (which I am willing to help you with at any point). |
|
@tmds - unfortunately, @0xced has been called away. So I'm going to put my name down on this, with an intention of getting the PR over the line. I'm about to pull down the branch and take a look. Short of more significant prompt refactoring @0xced had in mind, it looks like a few unit test changes is all that's required for this PR. Do you agree, and if so, are you able to help with them? I can review and merge, when appropriate. |
|
This is now top of my list to get over the line @tmds. I'll take a look at what unit test coverage we need, then it should be a trivial matter of updating your branch and pushing. Then we can welcome you into the |
|
hi @FrankRay78 , sorry for the unresponsiveness, it was a very busy time for me. If you can provide some guidance on the unit test coverage, we can try to get this merged by the end of next week. |
Unit tests for console.AskAsync, console.ConfirmAsync
|
I'm not sure why there is a merge conflict that needs resolving via the command line - I'm going to see if I can do this and force push to your branch @tmds (although I likely don't have permission to do so). However, if you get there first, please resolve and update this branch. Then I can merge and we can move on with life 😊 |
|
I'm traveling, but I'd wager the attributes for AOT that were added might be at fault. I don't have a computer but I'm happy to answer any questions or review |
|
Hello @phil-scott-78, do you know if as a maintainer, whether I should be able to resolve the conflict of another users fork/branch, then force push it back? Or if the user will always need to do that themselves? The contribution is fully reviewed and good to merge. |
|
@FrankRay78, I've done it in the past and I've had maintainers fix my stuff, but the submitting user needs to enable it on their end. At the end of the day, I always feel like as long as the user's original commit is still in the history plus a maintainer commit with the needed touch up, then it's all good. |
afaik it is enabled by default and you should be able to push to my branch. I've resolved the conflicts by merging in the I assume you're going to squash this PR when merging. If not, let me know if you want me to clean up the commit history. |
|
Well done @tmds - we got there!!! Welcome to the Spectre.Console git commit history! Any future contributions you want to make, tag me in and I'll ensure it gets looked at earlier than this one. Thanks mate. |
|
Thanks for your help, @FrankRay78! |
@patriksvensson ptal.
Please upvote 👍 this pull request if you are interested in it.