Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Finder] Add a method to check if any results were found#23471
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f76cec1
tod30a06a
CompareWhy not to use |
Because that will iterate through every single file that matches. If I only want to know if some exist or not, that's an overly expensive check |
nicolas-grekas commentedJul 11, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I remember a similar proposal that wasn't accepted, because the use case is not common enough. foreach ($finderas$thing) {// do things herebreak;} |
It may be only one extra line but a lot of the clarity is lost. You see a |
@duncan3dc, I'm not sure but I think you could try to use the if ($finder->getIterator->valid()) {} |
@voronkovich Not if the iterator is an |
I like this feature proposal ... and I recently needed it 😄 However, I don't like the proposed I'd prefer a more descriptive method name: |
I like the proposal too, and it can be useful, |
👍 for |
* | ||
* @return bool | ||
*/ | ||
public function found() |
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.
what aboutmatches()
maybe?
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.
I've changed it tohasResults()
now
*/ | ||
public function found() | ||
{ | ||
foreach ($this->getIterator() as $null) { |
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.
might as well updategetIterator()
to@return \Traversable|SplFileInfo[]
then, to indicate why we add this method. Currently we could rely ongetIterator()->valid()
i believe :)
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.
I see that as outside the scope of this change, I suggest opening a separate PR if you believe that should be changed
@symfony/deciders we need to make a decision here. Personally I'm "-0" :) |
I'm 👍 |
*/ | ||
public function hasResults() | ||
{ | ||
foreach ($this->getIterator() as $null) { |
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.
Minor suggestion:$null
may be confusing and in the future, it may be a reserved word in PHP. Could we use$_
as the name of this unused variable as we do in other parts of Symfony? Thanks!
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.
Sure, I wasn't aware of the convention, I've updated it to use$_
now
@@ -5,6 +5,7 @@ CHANGELOG | |||
----- | |||
* deprecated `Symfony\Component\Finder\Iterator\FilterIterator` | |||
* added Finder::hasResults() method to check if any results were found |
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.
there should be backticks around the method:
* added`Finder::hasResults()` method to check if any results were found
Thank you@duncan3dc. |
…nd (duncan3dc)This PR was merged into the 3.4 branch.Discussion----------[Finder] Add a method to check if any results were found| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MITIf I want to know if any results were found, but I don't want to start trawling through them, I have to do the rather ugly:```php$found = false;foreach ($finder as $thing) { $found = true; break;}if ($found) {```This PR enables the much more readable:```phpif ($finder->found()) {```This seemed like an obvious thing to me, so I suspect there might be a reason this doesn't exist already, but I couldn't find any previous discussion. If it'll be accepted then I'll glady create a docs PRCommits-------24dcb52 Add a method to check if any results were found
If I want to know if any results were found, but I don't want to start trawling through them, I have to do the rather ugly:
This PR enables the much more readable:
This seemed like an obvious thing to me, so I suspect there might be a reason this doesn't exist already, but I couldn't find any previous discussion. If it'll be accepted then I'll glady create a docs PR