-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Make FilePatternMatch.Stem non-nullable. #118410
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
Make FilePatternMatch.Stem non-nullable. #118410
Conversation
- Add an ArgumentNullException if stem is null in Success method. - Add tests for null checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request makes the FilePatternMatch.Stem property non-nullable to improve type safety. The change ensures that the Stem property always has a valid string value, preventing potential null reference exceptions when consumers use this property.
Key changes:
- Converted
Stemproperty from nullable string to non-nullable string - Added null validation in the constructor to enforce the contract
- Added comprehensive tests to verify null argument validation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| FilePatternMatch.cs | Changed Stem property to non-nullable and added null validation in constructor |
| PatternTestResult.cs | Added MemberNotNullWhen attribute to support nullable analysis when IsSuccessful is true |
| FilePatternMatchTests.cs | Added unit tests to verify ArgumentNullException is thrown for null path and stem parameters |
Comments suppressed due to low confidence (2)
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FilePatternMatchTests.cs:32
- The test should verify that the exception is thrown for the correct parameter name. Consider using Assert.Throws("path", () => new FilePatternMatch(null, "sub2/bar/baz/three.txt")) to ensure the parameter name is validated.
Assert.Throws<ArgumentNullException>(() => new FilePatternMatch(null, "sub2/bar/baz/three.txt"));
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FilePatternMatchTests.cs:38
- The test should verify that the exception is thrown for the correct parameter name. Consider using Assert.Throws("stem", () => new FilePatternMatch("sub2/bar/baz/three.txt", null)) to ensure the parameter name is validated.
Assert.Throws<ArgumentNullException>(() => new FilePatternMatch("sub2/bar/baz/three.txt", null));
|
@dotnet-policy-service agree company="Microsoft" |
jeffhandley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me; thanks, @MitchBodmer! I will mark this as a
breaking-change
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
|
@jeffhandley it looks like the build hit some unrelated failures. Is there something I should do here? |
|
Thanks, @MitchBodmer. No worries; I'll review the failures further and the build analysis shortly and will update the build analysis with the justification for override, and then I'll merge this after I create the breaking-change notification. I'll let you know if I need anything further from you on it. Thanks for the contribution! |
|
Breaking change documentation issue created: [Breaking change]: FilePatternMatch.Stem annotated as non-nullable (dotnet/docs#47914). PR is pending review from @artl93 for .NET 10 RC1 consideration. |
Implements #116982