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
Enable nullable reference types for Hosting #1761
base: main
Are you sure you want to change the base?
Conversation
bdc2fef
to
fb0f9df
Compare
@@ -107,7 +107,7 @@ public static IHostBuilder UseCommandHandler(this IHostBuilder builder, Type com | |||
&& invocation.ParseResult.CommandResult.Command is Command command | |||
&& command.GetType() == commandType) | |||
{ | |||
invocation.BindingContext.AddService(handlerType, c => c.GetService<IHost>().Services.GetService(handlerType)); | |||
invocation.BindingContext.AddService(handlerType, c => c.GetRequiredService<IHost>().Services.GetService(handlerType)); |
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.
Why this change? I would think the IHost
is a required service here.
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.
GetRequiredService
would return service and ensure the service is not null, GetService
would return null
when no service found, think we expect IHost
exists, so maybe better to use GetRequiredService
https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.serviceproviderserviceextensions.getrequiredservice?view=dotnet-plat-ext-6.0
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 raises a question about whether callers can expect this to have been configured or not. I suspect the answer is that they can and if it's not available, it could be because of a code defect.
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.
Think it should not be registered when IHost
or the handler is null
, maybe I could add a null check for that
var instance = hostModelBinder.CreateInstance(invocationContext.BindingContext); | ||
if (instance is IHost host) | ||
return host; | ||
return null; |
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.
I would think we should not allow null here.
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.
I think so before, from test code
command-line-api/src/System.CommandLine.Hosting.Tests/HostingTests.cs
Lines 401 to 403 in 209b724
IHost host = invCtx.GetHost(); | |
host.Should().BeNull(); | |
hostAsserted = true; |
it may be null, if this should not be
null
, guess we should throw an InvalidOperationException
?
Co-authored-by: Kevin B <Keboo@users.noreply.github.com>
Fixes #1760
commandline
=>commandLine
to fix typoToList
for configuration extension