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

[Group 3] Enable nullable annotations for Microsoft.Extensions.Hosting.Abstractions #65403

Merged
merged 8 commits into from
Mar 16, 2022

Conversation

maxkoshevoi
Copy link
Contributor

Related to #43605

This one is a bit tricky. A lot of abstractions, and hard to know what's null, and what isn't.

Questions:

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 15, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Feb 15, 2022

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #43605

This one is a bit tricky. A lot of abstractions, and hard to know what's null, and what isn't.

Questions:

Author: maxkoshevoi
Assignees: -
Labels:

new-api-needs-documentation, area-Extensions-Hosting, community-contribution

Milestone: -

{
Properties = properties;
}

/// <summary>
/// The <see cref="IHostEnvironment" /> initialized by the <see cref="IHost" />.
/// </summary>
public IHostEnvironment HostingEnvironment { get; set; }
public IHostEnvironment? HostingEnvironment { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two properties are always set (when we create the object):

_hostBuilderContext = new HostBuilderContext(Properties)
{
HostingEnvironment = _hostingEnvironment,
Configuration = _hostConfiguration
};

It feels wrong to have consumers need to check these for null, when they won't be.

Copy link
Contributor Author

@maxkoshevoi maxkoshevoi Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR.

Hm, why they are not part of constructor then?

Comment on lines 120 to 121
Microsoft.Extensions.FileProviders.IFileProvider ContentRootFileProvider { get; set; }
string ContentRootPath { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will probably have problems with these two in the Hosting PR, but we can deal with it then. I think the intention here is that these two properties are not null.

@eerhardt eerhardt self-assigned this Mar 1, 2022
@maxkoshevoi
Copy link
Contributor Author

@eerhardt Hi, I'm in Ukraine (currently relatively safe in Lviv), so cannot be as active. I'll try to address the comments when I can, but if I don't respond for tool long or disappear altogether, feel free to commit to my branches.

@eerhardt
Copy link
Member

eerhardt commented Mar 4, 2022

Ok - no problem at all, @maxkoshevoi. I'm glad to hear you are safe, as that is the most important thing.

We can definitely take over the PR if you don't have time. Thanks again for all your contributions.

@maxkoshevoi
Copy link
Contributor Author

We can definitely take over the PR if you don't have time.

For now, I do have time. It's just a heads up that I will be slower to respond, and may not be able to respond for some periods of time. Libraries that left are not that big, so it should be fine.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the work here @maxkoshevoi!

@eerhardt eerhardt merged commit 2be87c9 into dotnet:main Mar 16, 2022
@@ -26,7 +26,7 @@ public interface IHostingEnvironment
/// Gets or sets the name of the application. This property is automatically set by the host to the assembly containing
/// the application entry point.
/// </summary>
string ApplicationName { get; set; }
string? ApplicationName { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eerhardt This one seems odd to be nullable, similar to EnvironmentName above, this is set automatically by the host

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the current code, this annotation is correct as there are scenarios where it can be null:

var hostingEnvironment = new HostingEnvironment()
{
ApplicationName = hostConfiguration[HostDefaults.ApplicationKey],
EnvironmentName = hostConfiguration[HostDefaults.EnvironmentKey] ?? Environments.Production,
ContentRootPath = ResolveContentRootPath(hostConfiguration[HostDefaults.ContentRootKey], AppContext.BaseDirectory),
};
if (string.IsNullOrEmpty(hostingEnvironment.ApplicationName))
{
// Note GetEntryAssembly returns null for the net4x console test runner.
hostingEnvironment.ApplicationName = Assembly.GetEntryAssembly()?.GetName().Name;
}

@maxkoshevoi maxkoshevoi deleted the mk/43605-hosting-abstraction branch March 17, 2022 17:42
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…ng.Abstractions` (dotnet#65403)

* First pass (src)

* Update Microsoft.Extensions.Hosting.Abstractions.cs

* Update HostBuilderContext

* Values of Properties cannot be null
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Hosting community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants