Skip to content

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

Merged
merged 11 commits into from
May 28, 2025

Conversation

PranavSenthilnathan
Copy link
Member

Adds the preserveFilterOrdering flag to FileSystemGlobbing matcher. See #114720 for original PR.

/cc @kasperk81

Fixes #109408

@Copilot Copilot AI review requested due to automatic review settings May 23, 2025 12:55
Copy link
Contributor

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

Copy link
Contributor

@Copilot Copilot AI left a 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 to Matcher 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 the preserveFilterOrder flag; consider renaming it to preserveFilterOrder (or preserveFilterOrdering) 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()

{
_basePath = basePath;
_recorder = new FileSystemOperationRecorder();
_patternMatching = matcher;
_patternMatching = new Matcher(preserveFilterOrder: preserveFilterOrder);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_patternMatching = new Matcher(preserveFilterOrder: preserveFilterOrder);
_patternMatching = new Matcher(preserveFilterOrder);

Copy link
Member Author

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.

Comment on lines +15 to +17
protected internal abstract PatternTestResult MatchPatternContexts<TFileInfoBase>(
TFileInfoBase fileInfo,
Func<IPatternContext, TFileInfoBase, PatternTestResult> test);
Copy link
Member

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.

Suggested change
protected internal abstract PatternTestResult MatchPatternContexts<TFileInfoBase>(
TFileInfoBase fileInfo,
Func<IPatternContext, TFileInfoBase, PatternTestResult> test);
protected internal abstract PatternTestResult MatchPatternContexts(
FileSystemInfoBase fileSystemInfo,
Func<IPatternContext, PatternTestResult> test);

Copy link
Member Author

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.

@PranavSenthilnathan PranavSenthilnathan merged commit 16a5d90 into dotnet:main May 28, 2025
85 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ordered evaluation in FileSystemGlobbing Matcher
3 participants