[Group 2] Enable nullable annotations for Microsoft.Extensions.Configuration.Abstractions#57401
Conversation
ghost
added
community-contribution
|
Note regarding the 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. |
|
Tagging subscribers to this area: @maryamariyan, @safern |
maxkoshevoi
changed the title
Microsoft.Extensions.Configuration.Abstractions
|
Thanks for the PRs @maxkoshevoi, the nullable annotations effort for Microsoft.Extensions.* is targeted for 7.0, and since this week we're focusing on 6.0 bugs, there might be some delays with the reviews. |
|
I hoped, at least |
# Conflicts: # src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationExtensions.cs
# Conflicts: # src/libraries/Microsoft.Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.csproj # src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/Microsoft.Extensions.Configuration.Abstractions.csproj
...Extensions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.cs
Show resolved
Hide resolved
...nsions.Configuration.Abstractions/ref/Microsoft.Extensions.Configuration.Abstractions.csproj
Show resolved
Hide resolved
| /// <param name="path">The path.</param> | ||
| /// <returns>The original path minus the last individual segment found in it. Null if the original path corresponds to a top level node.</returns> | ||
| public static string GetParentPath(string path) | ||
| public static string? GetParentPath(string? path) |
There was a problem hiding this comment.
honestly I think path should be marked as non-null the doc suggests it should be a path also it's not intuitive what will happen when it's null. I do not feel strongly about this so fine with me if you think we should keep it as is
There was a problem hiding this comment.
I also don't have a strong opinion. One benefit of making path nullable is that user don't need additional null-check in order to call this method (if their variable may be null).
I didn't find any calls to this method here or in aspnet, so don't know how this method it used.
src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/IConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/IConfigurationProvider.cs
Show resolved
Hide resolved
| /// Gets or sets the section value. | ||
| /// </summary> | ||
| string Value { get; set; } | ||
| string? Value { get; set; } |
There was a problem hiding this comment.
if you decide to keep path as nullable arg in other places should this also accept nullable path?
There was a problem hiding this comment.
I don't think so. Path will never be set to null. All sections have a path.
Also all usages don't expect null in Path:
- https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationRootExtensions.cs#L28
- https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationExtensions.cs#L57
- https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Abstractions/src/ConfigurationExtensions.cs#L64
Regarding leaving path parameters as null. I'm not objecting removing ?. My only concern is that it will be harder to call those methods by requiring additional null-check on their side.
...nsions.Configuration.Abstractions/src/Microsoft.Extensions.Configuration.Abstractions.csproj
Show resolved
Hide resolved
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
I think these changes look good. Thanks @maxkoshevoi.
4 participants
Related to #43605, #54012
Annotated according to:
Microsoft.Extensions.Primitives#57395