Remove set handler overloads #2089
Conversation
src/System.CommandLine/Command.cs
Outdated
| /// <summary> | ||
| /// Sets a command's handler based on a <see cref="Func{Task,InvocationContext}"/>. | ||
| /// </summary> | ||
| public void SetHandler(Func<InvocationContext, CancellationToken, Task> handle) | ||
| => Handler = new AnonymousCommandHandler(handle); |
There was a problem hiding this comment.
The way AnonymousCommandHandler checks whether the delegate returned a Task<int> rather than just a Task is rather surprising. If that feature is going to be preserved, it should be documented here.
There was a problem hiding this comment.
Besides, the reference to Func{InvocationContext, Task} is not correct as there is a CancellationToken as well.
There was a problem hiding this comment.
Anyway the compiler is going to translate the cref to just "T:System.Func`2" so it isn't that useful to specify the type arguments in the cref. It would be more useful to state that it's for an async function.
There was a problem hiding this comment.
If that feature is going to be preserved, it should be documented here.
Our goal is to remove InvocationContext and have an int-returning delegate that accepts a ParseResult and CancellationToken (in case of the async handler)
Besides, the reference to Func{InvocationContext, Task} is not correct as there is a CancellationToken as well.
Good point, I've updated the comment.
There was a problem hiding this comment.
The way AnonymousCommandHandler checks whether the delegate returned a Task rather than just a Task is rather surprising.
This was in place to support some of the reflection-based approaches that had been in approach in beta1 and should no longer be needed.
There was a problem hiding this comment.
Does #2071 cover the removal of the Task<int> type check or should I file a separate issue?
IValueDescriptor<T>andBinderBase<T>Makes progress towards #2071
contributes to #1918
fixes #2000, fixes #1878 (there will only be one handler)