-
Notifications
You must be signed in to change notification settings - Fork 5k
Ordered evaluation in FileSystemGlobbing Matcher #115940
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
Ordered evaluation in FileSystemGlobbing Matcher #115940
Conversation
Tagging subscribers to this area: @dotnet/area-extensions-filesystem |
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 PR introduces a preserveFilterOrder
flag to the Matcher
class, enabling patterns to be applied in the order they were added. It updates test harnesses and adds new ordered/unordered test suites, and refactors the internal context to support ordered vs. include-first matching.
- Add
preserveFilterOrder
parameter and logic toMatcher
and its constructors - Refactor
MatcherContext
to delegate to new ordered (PreserveOrderCompositePatternContext
) or include-first contexts - Update existing tests to pass the ordering flag and add new
OrderedPatternMatchingTests
+ an unordered test case
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
FileSystemGlobbingTestContext.cs | Constructor now takes bool isOrdered to drive new Matcher(preserveFilterOrder:) |
PatternMatchingTests.cs | Replaced raw Matcher instantiation with ordered flag; added PreserveFilterOrderingFalse test |
OrderedPatternMatchingTests.cs | New test class exercising complex ordered include/exclude sequences |
Matcher.cs | Introduce _preserveFilterOrder , overloads, and switch to ordered vs. include-first pattern lists |
MatcherContext.cs (+ internal patterned contexts) | Refactor to use CompositePatternContext implementations for ordered vs. include-first |
Microsoft.Extensions.FileSystemGlobbing.csproj | Minor whitespace/BOM adjustment |
Comments suppressed due to low confidence (2)
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/TestUtility/FileSystemGlobbingTestContext.cs:19
- [nitpick] The parameter name
isOrdered
is ambiguous relative to thepreserveFilterOrder
flag; consider renaming it topreserveFilterOrder
(orpreserveFilterOrdering
) for clarity and consistency.
public FileSystemGlobbingTestContext(string basePath, bool isOrdered)
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/PatternMatchingTests.cs:440
- Only one scenario tests
preserveFilterOrder=false
; consider adding additional unordered test cases (e.g., multiple include/exclude combinations) to cover edge cases in the new behavior.
public void PreserveFilterOrderingFalse()
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/PatternMatchingTests.cs
Outdated
Show resolved
Hide resolved
{ | ||
_basePath = basePath; | ||
_recorder = new FileSystemOperationRecorder(); | ||
_patternMatching = matcher; | ||
_patternMatching = new Matcher(preserveFilterOrder: preserveFilterOrder); |
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.
_patternMatching = new Matcher(preserveFilterOrder: preserveFilterOrder); | |
_patternMatching = new Matcher(preserveFilterOrder); |
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.
This is needed because of the API shape and overload resolution. There's a Matcher(StringComparison)
overload that will get preferred without the explicit parameter name because it has no optional parameters. The compiler then complains that bool isn't convertible to StringComparison
.
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/PatternMatchingTests.cs
Outdated
Show resolved
Hide resolved
protected internal abstract PatternTestResult MatchPatternContexts<TFileInfoBase>( | ||
TFileInfoBase fileInfo, | ||
Func<IPatternContext, TFileInfoBase, PatternTestResult> test); |
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.
Not need to be generic.
protected internal abstract PatternTestResult MatchPatternContexts<TFileInfoBase>( | |
TFileInfoBase fileInfo, | |
Func<IPatternContext, TFileInfoBase, PatternTestResult> test); | |
protected internal abstract PatternTestResult MatchPatternContexts( | |
FileSystemInfoBase fileSystemInfo, | |
Func<IPatternContext, PatternTestResult> test); |
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.
The type argument's naming is confusing since we don't actually care that it's a file info. This is instead using a TState
pattern, so the implementation will just call the test
with their IPatternContext
and pass in this opaque state for the ride. It could also just be:
protected internal abstract PatternTestResult MatchPatternContexts(Func<IPatternContext, PatternTestResult> test);
which would force us to have a closure. I don't know if that matters so much here, but this method is from the existing code and I decided to just leave it as is.
Adds the
preserveFilterOrdering
flag to FileSystemGlobbing matcher. See #114720 for original PR./cc @kasperk81
Fixes #109408