[Group 1] Enable nullable annotations for Microsoft.Extensions.FileSystemGlobbing#57398
Conversation
|
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, @dotnet/area-extensions-filesystem Issue DetailsRelated to #43605 Questions:
|
Microsoft.Extensions.FileSystemGlobbing
| if (segment is RecursiveWildcardSegment) | ||
| { | ||
| if (segmentsPatternStartsWith == null) | ||
| if (segmentsPatternStartsWith == null || segmentsPatternEndsWith == null || segmentsPatternContains == null) |
There was a problem hiding this comment.
I don't think we should be changing behavior here.
| } | ||
|
|
||
| if (segmentsPatternStartsWith == null) | ||
| if (segmentsPatternStartsWith == null || segmentsPatternEndsWith == null || segmentsPatternContains == null) |
There was a problem hiding this comment.
I don't think we should be changing behavior here. We will probably need to annotate RaggedPattern to allow for null segmentsPatternEndsWith and segmentsPatternContains, if they can't be proven to not be null here.
There was a problem hiding this comment.
They are initialized together, so only one of them is checked (if one is null - all of them are and vise versa). Compiler just doesn't know that.
I've added those checks just to let compiler know that other two lists are also not null.
Replaced additional checks with !
! need to be addressed
# Conflicts: # src/libraries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.csproj # src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Microsoft.Extensions.FileSystemGlobbing.csproj
|
@krwq @maxkoshevoi - is this PR ready to be merged? Or are there more changes needed? |
|
All good from my side |
|
@maxkoshevoi sorry for late re-review. The comment about hash is still valid, I've responded |
eerhardt
left a comment
There was a problem hiding this comment.
I just had one last comment. After that is addressed I think we can merge this.
|
@eerhardt resolved the comment |
|
Test failure is unrelated. #60088 |
Related to #43605
Questions:
DirectoryInfoBasediffer in nullability forGetDirectoryandGetFile. So marked both of them as nullable in interface. Issue is that interface documentation says that they shouldn't be null.Issue to fix nullability issues: #58211