Ordered evaluation in FileSystemGlobbing Matcher#114720
Ordered evaluation in FileSystemGlobbing Matcher#114720kasperk81 wants to merge 0 commit intodotnet:mainfrom
Conversation
|
Note regarding the |
1 similar comment
|
Note regarding the |
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/MatcherContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/PatternMatchingTests.cs
Outdated
Show resolved
Hide resolved
|
@jeffhandley @ericstj @jozkee please take a look at this approved API implementation so it doesn't miss the .NET 10 release. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for ordered filter evaluation in the FileSystemGlobbing matcher by introducing a flag, updating the matching logic, and expanding tests accordingly.
- Introduce
preserveFilterOrderflag and overloads inMatcher - Implement ordered traversal in
MatcherContextviaMatchOrdered - Update existing tests to pass an
IsOrderedflag and addOrderedPatternMatchingTests
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| FileSystemGlobbingTestContext.cs | Constructor now takes isOrdered and creates matcher accordingly |
| PatternMatchingTests.cs | Updated to use IsOrdered instead of injecting a Matcher |
| OrderedPatternMatchingTests.cs | New test class exercising ordered filter behavior |
| Matcher.cs | Added preserveFilterOrder flag and switched execution paths |
| MatcherContext.cs | Extended to support ordered matching with local functions |
| InMemoryDirectoryInfo.cs | Modernized collection initialization to use new array syntax |
| Microsoft.Extensions.FileSystemGlobbing.csproj | Added System.ValueTuple package reference for .NET Framework |
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/PatternMatchingTests.cs:12
- [nitpick] Fields in C# tests typically use camelCase or are declared as properties. Consider renaming
IsOrderedto_isOrderedor exposing it as a protected propertyprotected bool IsOrdered { get; set; }to follow naming conventions.
protected bool IsOrdered = false;
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Matcher.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/MatcherContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Matcher.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Matcher.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Matcher.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.Extensions.FileSystemGlobbing/src/Microsoft.Extensions.FileSystemGlobbing.csproj
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/PatternMatchingTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/OrderedPatternMatchingTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/MatcherContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/MatcherContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/MatcherContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Internal/MatcherContext.cs
Show resolved
Hide resolved
|
@PranavSenthilnathan feel free to take over and adjust this as you see fit. The PR is functionally complete, and I addressed all your feedback a month ago but now you're requesting a complete overhaul? @jkotas could I get an authoritative review here? I'd like to avoid going back and forth with shifting feedback. |
You have the right people CCed here. I am not the right person to ask for an authoritative review on this one. https://github.com/dotnet/runtime/blob/main/docs/area-owners.md has the list of folks and aliases to tag for each area. |
|
Not sure why pushing to kasperk81:main automatically closed this PR... I'll create a new PR with the changes I tried to push. |
Recommend review with whitespace diffs off because converting ordered/unordered helpers to local functions caused indentation changes.
Resolve #109408