Allow overriding the default file name pattern for Discovery#292
Open
johnhunt-lc wants to merge 8 commits intomodelcontextprotocol:mainfrom
Open
Allow overriding the default file name pattern for Discovery#292johnhunt-lc wants to merge 8 commits intomodelcontextprotocol:mainfrom
johnhunt-lc wants to merge 8 commits intomodelcontextprotocol:mainfrom
Conversation
… scan for `*.php` files can be overridden.
…t autoloads from a non .php file for the test)
chr-hertel
requested changes
Apr 26, 2026
Member
chr-hertel
left a comment
There was a problem hiding this comment.
Hey @johnhunt-lc, wasn't aware of that use case, but recall seeing that convention years ago. Thanks for the patch, minor comments tho.
| * @param array<string>|null $namePatterns list of file name patterns for the scan. Compatible with Finder->name() | ||
| */ | ||
| public function discover(string $basePath, array $directories, array $excludeDirs = []): DiscoveryState | ||
| public function discover(string $basePath, array $directories, array $excludeDirs = [], ?array $namePatterns = null): DiscoveryState |
Member
There was a problem hiding this comment.
Using a default here would ease the change:
Suggested change
| public function discover(string $basePath, array $directories, array $excludeDirs = [], ?array $namePatterns = null): DiscoveryState | |
| public function discover(string $basePath, array $directories, array $excludeDirs = [], array $namePatterns = ['*.php']): DiscoveryState |
Author
There was a problem hiding this comment.
I was batting around different ideas for this. $namePatterns gets passed through quite a few different functions, and I originally specified the ['*.php'] default for each one, but I realized that was scattering hardcoded defaults everywhere so I switched to default null in all the functions that pass it along, and just set the default in the ternary below.
I agree that I prefer having the default in the params instead. Would you rather I:
- set a class const and use it for the default in all functions that use this argument?
- hardcode this in all of them?
- set it here, make the rest default to null, make this nullable, and handle nulls in the function logic?
- some other 4th thing I haven't thought of?
Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the option to pass in a
namePatternsarray of patterns to match against filenames, overriding the default'*.php'pattern that was previously hardcoded.Motivation and Context
It would be useful to be able to either limit further, or expand, the scope of files matched by discovery. The specific cases I've run into that require this functionality are:
*.incfor example)How Has This Been Tested?
I have tested this in a php application that uses
.incfiles to store classes, and have added additional unit tests. I haven't tried it in other contexts.Breaking Changes
This should not affect existing code.
Types of changes
Checklist
Additional context