Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

WeihanLi
Copy link

@WeihanLi WeihanLi commented Jun 12, 2022

Fixes #1760

  • Enable nullable reference types for Hosting
  • Renaming commandline => commandLine to fix typo
  • Remove unnecessary ToList for configuration extension

@WeihanLi WeihanLi force-pushed the hosting-nullable-reference-types branch from bdc2fef to fb0f9df Compare June 12, 2022 03:52
src/System.CommandLine.Hosting/Directory.Build.props Outdated Show resolved Hide resolved
@@ -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));
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Contributor

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.

Copy link
Author

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;
Copy link
Contributor

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.

Copy link
Author

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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hosting Nullable reference types support
3 participants