command-line-api icon indicating copy to clipboard operation
command-line-api copied to clipboard

Consider adding BindingContext to HelpContext/CompletionContext

Open baronfel opened this issue 3 years ago • 2 comments

While updating dotnet/templating to use System.CommandLine and incorporating that work into dotnet/sdk, we discovered a few gaps that we think require the addition of the BindingContext in order to solve.

The overall relationship between templating and the sdk is that templating exposes a factory function to create a System.CommandLine.Command representing dotnet new and all of its subcommands, options, and arguments. The intent is that this command can be then seamlessly plugged in to the dotnet root command in dotnet/sdk. One thing that we found while trialing this architecture is that the model we initially used provided all of the dependencies of the NewCommand up-front, instead of providing a Binder or resolving the dependencies from the ServiceProvider on the InvocationContext. When we started to move to this kind of a model, we found that Help and Completions were not able to be solved in this way. This is because the Help and Completions for dotnet new are very dynamic and resolved from templates, which means that the HelpContext and CompletionContext need a type that's responsible for providing this information. This would be ideal to retrieve from DI, but neither the HelpContext nor the CompletionContext have the BindingContext available on them.

Workarounds include manually re-implementing the help middleware and completion middleware and using our own HelpResult and CompletionResult subclasses that we do pass the necessary contexts to, but that doesn't compose especially well - the SDK would need to have some kind of meta-result that could delegate to specific sub-commands' result types in order to enable this I think.

baronfel avatar Dec 23 '21 16:12 baronfel

The same problems arise for the option validation and option default value. Those need to be both async and have access to the binding context to resolve transient services.

It's probably fair to say that all delegates in the api will require this.

justinmchase avatar Sep 29 '23 15:09 justinmchase

For resolving services, I suppose a workaround would be to set up a static AsyncLocal<T> where T is a mutable object with an IServiceProvider property. This would flow in ExecutionContext to the validators. After the invocation finishes, null out the IServiceProvider property of the T instance, so that the IServiceProvider can be garbage-collected even if the T instance is still referenced via an ExecutionContext that was inadvertently captured in some long-lived task that was started during parsing, validation, or invocation. This would be somewhat ugly but I think it would get the job done.

KalleOlaviNiemitalo avatar Sep 29 '23 17:09 KalleOlaviNiemitalo