Uh oh!
There was an error while loading. Please reload this page.
[Group 2] Enable nullable annotations for Microsoft.Extensions.Configuration.Abstractions - #57401
Conversation
ghost
commented
Aug 14, 2021
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. |
ghost
commented
Aug 14, 2021
Tagging subscribers to this area: @maryamariyan, @safern |
Microsoft.Extensions.Configuration.Abstractionsmaryamariyan
commented
Aug 16, 2021
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. |
maxkoshevoi
commented
Aug 17, 2021
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /// <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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /// 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.
Uh oh!
There was an error while loading. Please reload this page.
eerhardt
left a comment
There was a problem hiding this comment.
I think these changes look good. Thanks @maxkoshevoi.
Related to #43605, #54012
Annotated according to:
Microsoft.Extensions.Primitives#57395