Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:3.4fromduncan3dc:finder-found
Sep 26, 2017

Conversation

duncan3dc
Copy link
Contributor

QA
Branch?3.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT

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 ($finderas$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

voronkovich reacted with thumbs up emoji
@duncan3dcduncan3dc changed the base branch frommaster to3.4July 10, 2017 21:40
@duncan3dcduncan3dcforce-pushed thefinder-found branch 2 times, most recently fromf76cec1 tod30a06aCompareJuly 10, 2017 21:46
@Koc
Copy link
Contributor

Koc commentedJul 10, 2017

Why not to usecount($finder)?

@duncan3dc
Copy link
ContributorAuthor

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
Copy link
Member

nicolas-grekas commentedJul 11, 2017
edited
Loading

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

foreach ($finderas$thing) {// do things herebreak;}

@nicolas-grekasnicolas-grekas added this to the3.4 milestoneJul 11, 2017
@duncan3dc
Copy link
ContributorAuthor

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

apfelbox, Simperfit, voronkovich, ostrolucky, and robfrawley reacted with thumbs up emoji

@voronkovich
Copy link
Contributor

@duncan3dc, I'm not sure but I think you could try to use theIterator::valid method instead offoreach loop:

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

@duncan3dc
Copy link
ContributorAuthor

@voronkovich Not if the iterator is anIteratorAggregate you can't

Koc, apfelbox, and voronkovich reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

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

However, I don't like the proposedfound() method name, so I checked the method names ofPHP SPL iterators. They don't have a method to check if the iterator has elements, but somewhat related it's thevalid() 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.

duncan3dc reacted with thumbs up emoji

@Simperfit
Copy link
Contributor

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

@voronkovich
Copy link
Contributor

👍 forhasResults

ro0NL and alamirault reacted with thumbs up emoji

*
* @return bool
*/
public function found()
Copy link
Contributor

Choose a reason for hiding this comment

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

what aboutmatches() maybe?

Copy link
ContributorAuthor

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) {
Copy link
Contributor

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 :)

Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

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

@javiereguiluz
Copy link
Member

I'm 👍

*/
public function hasResults()
{
foreach ($this->getIterator() as $null) {

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!

Copy link
ContributorAuthor

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
Copy link
Member

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

@fabpot
Copy link
Member

Thank you@duncan3dc.

duncan3dc reacted with heart emoji

@fabpotfabpot merged commit24dcb52 intosymfony:3.4Sep 26, 2017
fabpot added a commit that referenced this pull requestSep 26, 2017
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ro0NLro0NLro0NL left review comments

@fabpotfabpotfabpot approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@xabbuhxabbuhxabbuh approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.

10 participants
@duncan3dc@Koc@nicolas-grekas@voronkovich@javiereguiluz@Simperfit@fabpot@ro0NL@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp