Skip to content

Ordered evaluation in FileSystemGlobbing Matcher #114720

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

Closed
wants to merge 0 commits into from

Conversation

kasperk81
Copy link
Contributor

Recommend review with whitespace diffs off because converting ordered/unordered helpers to local functions caused indentation changes.

Resolve #109408

@ghost
Copy link

ghost commented Apr 15, 2025

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, 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.

1 similar comment
@ghost
Copy link

ghost commented Apr 15, 2025

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, 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.

@kasperk81
Copy link
Contributor Author

@jeffhandley @ericstj @jozkee please take a look at this approved API implementation so it doesn't miss the .NET 10 release.

@ericstj ericstj requested a review from Copilot May 22, 2025 15:52
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 adds support for ordered filter evaluation in the FileSystemGlobbing matcher by introducing a flag, updating the matching logic, and expanding tests accordingly.

  • Introduce preserveFilterOrder flag and overloads in Matcher
  • Implement ordered traversal in MatcherContext via MatchOrdered
  • Update existing tests to pass an IsOrdered flag and add OrderedPatternMatchingTests

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 IsOrdered to _isOrdered or exposing it as a protected property protected bool IsOrdered { get; set; } to follow naming conventions.
protected bool IsOrdered = false;

@kasperk81
Copy link
Contributor Author

@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.

@jkotas
Copy link
Member

jkotas commented May 22, 2025

@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.

@PranavSenthilnathan
Copy link
Member

PranavSenthilnathan commented May 23, 2025

Not sure why pushing to kasperk81:main automatically closed this PR... I'll create a new PR with the changes I tried to push.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-FileSystem community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ordered evaluation in FileSystemGlobbing Matcher
3 participants