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.

Sign up for GitHub

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

Jump to bottom

[Group 1] Enable nullable annotations for Microsoft.Extensions.FileSystemGlobbing #57398

Merged
merged 25 commits into from
Oct 6, 2021
Merged

[Group 1] Enable nullable annotations for Microsoft.Extensions.FileSystemGlobbing #57398

merged 25 commits into from
Oct 6, 2021

Conversation

maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Aug 14, 2021 •
edited
Loading

Related to #43605

Questions:

  • Implementations of DirectoryInfoBase differ in nullability for GetDirectory and GetFile. 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

@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 ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 14, 2021
@ghost
Copy link

ghost commented Aug 14, 2021

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

Issue Details

Related to #43605

Questions:

  • Implementations of DirectoryInfoBase differ in nullability for GetDirectory and GetFile. So marked both of them as nullable in interface. Issue is that interface documentation says that they shouldn't be null.
Author: maxkoshevoi
Assignees: -
Labels:

new-api-needs-documentation, area-Extensions-FileSystem
Milestone: -

@maxkoshevoi maxkoshevoi changed the title Enable nullable annotations for Microsoft.Extensions.FileSystemGlobbing [Group 1] Enable nullable annotations for Microsoft.Extensions.FileSystemGlobbing Aug 15, 2021
@adamsitnik adamsitnik added this to the 7.0.0 milestone Aug 16, 2021
eerhardt
eerhardt reviewed Aug 25, 2021
View reviewed changes
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/FilePatternMatch.cs Show resolved Hide resolved
...aries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs Outdated Show resolved Hide resolved
...aries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.cs Show resolved Hide resolved
...aries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/Patterns/PatternBuilder.cs Outdated
@@ -176,7 +176,7 @@ public IPattern Build(string pattern)
{
if (segment is RecursiveWildcardSegment)
{
if (segmentsPatternStartsWith == null)
if (segmentsPatternStartsWith == null || segmentsPatternEndsWith == null || segmentsPatternContains == null)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be changing behavior here.

src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/Patterns/PatternBuilder.cs Outdated
@@ -199,7 +199,7 @@ public IPattern Build(string pattern)
scanPattern = endSegment + 1;
}

if (segmentsPatternStartsWith == null)
if (segmentsPatternStartsWith == null || segmentsPatternEndsWith == null || segmentsPatternContains == null)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 !

...aries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.cs Outdated Show resolved Hide resolved
...aries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.cs Show resolved Hide resolved
...aries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.cs Show resolved Hide resolved
@runfoapp runfoapp bot mentioned this pull request Aug 27, 2021
Closed
@maxkoshevoi
Update FilePatternMatchTests.cs
afb6ac8
@maxkoshevoi
Merge branch 'main' into mk/43605-FileSystemGlobbing
af3bebf 
# Conflicts:
#	src/libraries/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.csproj
#	src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Microsoft.Extensions.FileSystemGlobbing.csproj
krwq
krwq approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM with 2 comments

@eerhardt
Copy link
Member

eerhardt commented Oct 4, 2021

@krwq @maxkoshevoi - is this PR ready to be merged? Or are there more changes needed?

@maxkoshevoi
Copy link
Contributor Author

All good from my side

@krwq
Copy link
Member

krwq commented Oct 5, 2021

@maxkoshevoi sorry for late re-review. The comment about hash is still valid, I've responded

@krwq krwq mentioned this pull request Oct 5, 2021
Closed
36 tasks
@maxkoshevoi
Copy link
Contributor Author

@krwq @eerhardt Removed the test. PR can be merged now.

@runfoapp runfoapp bot mentioned this pull request Oct 5, 2021
Closed
eerhardt
eerhardt approved these changes Oct 5, 2021
View reviewed changes
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.

I just had one last comment. After that is addressed I think we can merge this.

@maxkoshevoi
Copy link
Contributor Author

@eerhardt resolved the comment

@eerhardt eerhardt mentioned this pull request Oct 6, 2021
Closed
@eerhardt
Copy link
Member

eerhardt commented Oct 6, 2021

Test failure is unrelated. #60088

@eerhardt eerhardt merged commit 1c30027 into dotnet:main Oct 6, 2021
@maxkoshevoi maxkoshevoi deleted the mk/43605-FileSystemGlobbing branch October 6, 2021 21:29
@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2021
@jeffhandley jeffhandley linked an issue Aug 9, 2022 that may be closed by this pull request
Remove ! from Microsoft.Extensions.FileSystemGlobbing #58211
Closed
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krwq krwq krwq approved these changes

@eerhardt eerhardt eerhardt approved these changes

@maryamariyan maryamariyan Awaiting requested review from maryamariyan

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

@jozkee jozkee Awaiting requested review from jozkee

@adamsitnik adamsitnik Awaiting requested review from adamsitnik

Assignees
No one assigned
Labels
area-Extensions-FileSystem community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

Remove ! from Microsoft.Extensions.FileSystemGlobbing
4 participants
@maxkoshevoi @eerhardt @krwq @adamsitnik