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

[Finder] Add a method to check if any results were found #23471

Merged
merged 1 commit into from Sep 26, 2017

Conversation

Projects
None yet
10 participants
@duncan3dc
Contributor

duncan3dc commented Jul 10, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

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:

$found = false;
foreach ($finder as $thing) {
    $found = true;
    break;
}
if ($found) {

This PR enables the much more readable:

if ($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 PR

@duncan3dc duncan3dc changed the base branch from master to 3.4 Jul 10, 2017

@Koc

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc Jul 10, 2017

Contributor

Why not to use count($finder)?

Contributor

Koc commented Jul 10, 2017

Why not to use count($finder)?

@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Jul 10, 2017

Contributor

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

Contributor

duncan3dc commented Jul 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 11, 2017

Member

I remember a similar proposal that wasn't accepted, because the use case is not common enough.
You can achieve it in just one extra line btw:

foreach ($finder as $thing) {
    // do things here
    break;
}
Member

nicolas-grekas commented Jul 11, 2017

I remember a similar proposal that wasn't accepted, because the use case is not common enough.
You can achieve it in just one extra line btw:

foreach ($finder as $thing) {
    // do things here
    break;
}

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 11, 2017

@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Jul 11, 2017

Contributor

It may be only one extra line but a lot of the clarity is lost. You see a foreach and you think we're doing something with the results, it's misleading

Contributor

duncan3dc commented Jul 11, 2017

It may be only one extra line but a lot of the clarity is lost. You see a foreach and you think we're doing something with the results, it's misleading

@voronkovich

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Jul 11, 2017

Contributor

@duncan3dc, I'm not sure but I think you could try to use the Iterator::valid method instead of foreach loop:

if ($finder->getIterator->valid()) {
}
Contributor

voronkovich commented Jul 11, 2017

@duncan3dc, I'm not sure but I think you could try to use the Iterator::valid method instead of foreach loop:

if ($finder->getIterator->valid()) {
}
@duncan3dc

This comment has been minimized.

Show comment
Hide comment
@duncan3dc

duncan3dc Jul 11, 2017

Contributor

@voronkovich Not if the iterator is an IteratorAggregate you can't

Contributor

duncan3dc commented Jul 11, 2017

@voronkovich Not if the iterator is an IteratorAggregate you can't

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 12, 2017

Member

I like this feature proposal ... and I recently needed it 😄

However, I don't like the proposed found() method name, so I checked the method names of PHP SPL iterators. They don't have a method to check if the iterator has elements, but somewhat related it's the valid() method (e.g. in ArrayIterator, valid() returns true if the array contains any more entries).

I'd prefer a more descriptive method name: hasResults(), hasItems(), hasEntries(), hasElements(), etc.

Member

javiereguiluz commented Jul 12, 2017

I like this feature proposal ... and I recently needed it 😄

However, I don't like the proposed found() method name, so I checked the method names of PHP SPL iterators. They don't have a method to check if the iterator has elements, but somewhat related it's the valid() method (e.g. in ArrayIterator, valid() returns true if the array contains any more entries).

I'd prefer a more descriptive method name: hasResults(), hasItems(), hasEntries(), hasElements(), etc.

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Jul 12, 2017

Contributor

I like the proposal too, and it can be useful, hasResults() is quite nice

Contributor

Simperfit commented Jul 12, 2017

I like the proposal too, and it can be useful, hasResults() is quite nice

@voronkovich

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Jul 12, 2017

Contributor

👍 for hasResults

Contributor

voronkovich commented Jul 12, 2017

👍 for hasResults

Show outdated Hide outdated src/Symfony/Component/Finder/Finder.php
*
* @return bool
*/
public function found()

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

what about matches() maybe?

@ro0NL

ro0NL Jul 14, 2017

Contributor

what about matches() maybe?

This comment has been minimized.

@duncan3dc

duncan3dc Jul 15, 2017

Contributor

I've changed it to hasResults() now

@duncan3dc

duncan3dc Jul 15, 2017

Contributor

I've changed it to hasResults() now

Show outdated Hide outdated src/Symfony/Component/Finder/Finder.php
*/
public function found()
{
foreach ($this->getIterator() as $null) {

This comment has been minimized.

@ro0NL

ro0NL Jul 14, 2017

Contributor

might as well update getIterator() to @return \Traversable|SplFileInfo[] then, to indicate why we add this method. Currently we could rely on getIterator()->valid() i believe :)

@ro0NL

ro0NL Jul 14, 2017

Contributor

might as well update getIterator() to @return \Traversable|SplFileInfo[] then, to indicate why we add this method. Currently we could rely on getIterator()->valid() i believe :)

This comment has been minimized.

@duncan3dc

duncan3dc Jul 15, 2017

Contributor

I see that as outside the scope of this change, I suggest opening a separate PR if you believe that should be changed

@duncan3dc

duncan3dc Jul 15, 2017

Contributor

I see that as outside the scope of this change, I suggest opening a separate PR if you believe that should be changed

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 26, 2017

Member

@symfony/deciders we need to make a decision here. Personally I'm "-0" :)

Member

nicolas-grekas commented Sep 26, 2017

@symfony/deciders we need to make a decision here. Personally I'm "-0" :)

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 26, 2017

Member

I'm 👍

Member

javiereguiluz commented Sep 26, 2017

I'm 👍

Show outdated Hide outdated src/Symfony/Component/Finder/Finder.php
*/
public function hasResults()
{
foreach ($this->getIterator() as $null) {

This comment has been minimized.

@javiereguiluz

javiereguiluz Sep 26, 2017

Member

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!

@javiereguiluz

javiereguiluz Sep 26, 2017

Member

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!

This comment has been minimized.

@duncan3dc

duncan3dc Sep 26, 2017

Contributor

Sure, I wasn't aware of the convention, I've updated it to use $_ now

@duncan3dc

duncan3dc Sep 26, 2017

Contributor

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

This comment has been minimized.

@xabbuh

xabbuh Sep 26, 2017

Member

there should be backticks around the method:

 * added `Finder::hasResults()` method to check if any results were found
@xabbuh

xabbuh Sep 26, 2017

Member

there should be backticks around the method:

 * added `Finder::hasResults()` method to check if any results were found
@xabbuh

xabbuh approved these changes Sep 26, 2017

@fabpot

fabpot approved these changes Sep 26, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 26, 2017

Member

Thank you @duncan3dc.

Member

fabpot commented Sep 26, 2017

Thank you @duncan3dc.

@fabpot fabpot merged commit 24dcb52 into symfony:3.4 Sep 26, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 26, 2017

feature #23471 [Finder] Add a method to check if any results were fou…
…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       | MIT

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:
```php
$found = false;
foreach ($finder as $thing) {
    $found = true;
    break;
}
if ($found) {
```
This PR enables the much more readable:
```php
if ($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 PR

Commits
-------

24dcb52 Add a method to check if any results were found

@duncan3dc duncan3dc deleted the duncan3dc:finder-found branch Sep 27, 2017

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment