Conversation
|
|
||
| var command = new Command("hello"); | ||
| command.SetHandler((InvocationContext ctx) => | ||
| command.SetHandler(ctx => |
There was a problem hiding this comment.
it's really nice for the simple handlers to be naturally-typed here (and in the other 1000 callsites in this PR)
There was a problem hiding this comment.
Agreed. Settling on InvocationContext for the case where we do this (i.e. removing support for ParseResult, etc. as parameters) reduces a lot of DI-ish complexity in these handlers. If we pull this thread hard enough, the need for the service provider might disappear.
baronfel
left a comment
baronfel
left a comment
There was a problem hiding this comment.
Nothing major, but man those simple callsites look a lot nicer.
Keboo
left a comment
Keboo
left a comment
There was a problem hiding this comment.
Looks good. Might be worth reviewing System.CommandLine.Generator after this merges, as I suspect it could be cleaned up a little bit to leverage the InvocationContext SetHandler overload.
| { | ||
| var valueSource = ValueDescriptorDefaultValueSource.Instance; | ||
|
|
||
| valueSource.TryGetValue(valueDescriptor, context, out var value); |
There was a problem hiding this comment.
nit: I dislike var here as it is not clear what the type of value is.
| { | ||
| var valueSource = ValueDescriptorDefaultValueSource.Instance; | ||
|
|
||
| valueSource.TryGetValue(valueDescriptor, context, out var value); |
There was a problem hiding this comment.
Should we care about the case where TryGetValue returns false? Should there be something line BoundValue.None to return in that case?
3 participants
These changes attempt to improve the clarity of the
SetHandlermethods by removing theparamsparameters and replacing them with a number ofIValueDescriptor<T>parameters matching the number of generic type arguments.Since DI-like behaviors are fairly non-obvious, I'll likely also experiment here with removing the internal service provider and see if
BinderBase<T>can fulfill its role.