-
-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support configure the app with generic host #363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to copy the behavior of the non-generic CommandLineApplication
to allow external configuration!
/// <param name="cancellationToken">A cancellation token</param> | ||
/// <returns>A task whose result is the exit code of the application</returns> | ||
public static async Task<int> RunCommandLineApplicationAsync<TApp>( | ||
this IHostBuilder hostBuilder, string[] args, CancellationToken cancellationToken = default) | ||
this IHostBuilder hostBuilder, string[] args, Action<CommandLineApplication> configure = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs a separate overload so that this isn’t a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense
Any objection pls? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening a pull request! Sorry for delayed response. I have one suggestion, but otherwise this looks fine to me.
Also, any chance you can add a unit test to verify the behavior?
Thanks,
Nate
public CommandLineService(ILogger<CommandLineService<T>> logger, CommandLineState state, | ||
IServiceProvider serviceProvider) | ||
IServiceProvider serviceProvider, Action<CommandLineApplication> configure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IServiceProvider serviceProvider, Action<CommandLineApplication> configure) | |
IServiceProvider serviceProvider, Action<CommandLineApplication<T>> configure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is the _application
is CommandLineApplication
but not CommandLineApplication<T>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we cannot update _application to the generic type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks very strange to me.
_application = new CommandLineApplication<T>(state.Console, state.WorkingDirectory)
I thought the _application is a generic one but Visual Studio tells me it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. I forgot the variable declaration.
public static async Task<int> RunCommandLineApplicationAsync<TApp>( | ||
this IHostBuilder hostBuilder, | ||
string[] args, | ||
Action<CommandLineApplication> configure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Action<CommandLineApplication> configure, | |
Action<CommandLineApplication<TApp>> configure, |
If we make this use the generic parameter, then users can also access the Model
and ModelFactory
properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @scott-xu!
This PR should fix #309
and fix #358