Skip to content
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] document array use for locations #6917

Merged
merged 1 commit into from
Sep 22, 2016
Merged

Conversation

mickaelandrieu
Copy link
Contributor

@mickaelandrieu mickaelandrieu commented Aug 25, 2016

Hi,

This pull request is valid from Symfony 2.7 to 3.2-dev.

The function Finder::in() accepts a string or array of string for look into multiple locations => https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Finder/Finder.php

The other way described is valid, but shouldn't be recommended/documented.

Mickaël

@@ -82,7 +82,7 @@ directory to use for the search::
Search in several locations by chaining calls to
Copy link
Member

@wouterj wouterj Aug 25, 2016

Choose a reason for hiding this comment

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

this no longer holds true.

I propose to merge the code examples (this one and the ones on line 80 & 89):

The location is the only mandatory criteria. It tells the finder which
directory to use for the search::

    $finder->in(__DIR__);

    // search in several locations
    $finder->in(array(__DIR__, '/elsewhere'));
    $finder->in(__DIR__)->in('/elsewhere');

    // use wildcard characters (it has to resolve to at least one directory path)
    $finder->in('src/Symfony/*/*/Resources');

Exclude directories from matching with the [...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it's less readable...

Is it ok for you to suggest the 2 options ? See my update :)

Copy link
Member

Choose a reason for hiding this comment

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

👍 for Wouter's suggestion.

@mickaelandrieu we're including more code, with short comments to explain them, as we've found that people mostly scan to code-blocks anyways. This is a situation where this isn't a huge win, because the text between the paragraphs is so short, but I still like.

Copy link
Member

Choose a reason for hiding this comment

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

Except for one change:

// search inside *both* directories
$finder->in(array(__DIR__, '/elsewhere'));
// same as above
$finder->in(__DIR__)->in('/elsewhere');

This is to be clear what calling in() twice does - it does not replace the previous directory.

@wouterj
Copy link
Member

wouterj commented Aug 25, 2016

Thanks ,makes sense. We should still keep the reference to chaining though (e.g. when dynamically adding multiple directories), see my proposal.

Status: needs work

@stof
Copy link
Member

stof commented Aug 26, 2016

multiple calls to ->in() are handy when you make these calls conditionally.

Thus, it is good to explain to the user what happens when calling it twice (some people might expect that it replaces the previous value entirely)

@weaverryan
Copy link
Member

👍 for Wouter's version of this

Status: Reviewed

@xabbuh xabbuh merged commit bff0ce6 into symfony:2.7 Sep 22, 2016
xabbuh added a commit that referenced this pull request Sep 22, 2016
This PR was merged into the 2.7 branch.

Discussion
----------

[Finder] document array use for locations

Hi,

This pull request is valid from Symfony 2.7 to 3.2-dev.

The function ``Finder::in()`` accepts a string or array of string for look into multiple locations => https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Finder/Finder.php

The other way described is valid, but shouldn't be recommended/documented.

Mickaël

Commits
-------

bff0ce6 [Finder] document array use for locations
xabbuh added a commit that referenced this pull request Sep 22, 2016
@xabbuh
Copy link
Member

xabbuh commented Sep 22, 2016

Thank you @mickaelandrieu. This is now merged. I also made a little tweak suggested by @weaverryan in 232ebc3.

@mickaelandrieu mickaelandrieu deleted the patch-5 branch September 22, 2016 09:34
xabbuh added a commit that referenced this pull request Sep 22, 2016
* 2.7:
  Several typo fixes
  Update console.rst
  [#6917] some tweaks after review
  [Finder] document array use for locations
xabbuh added a commit that referenced this pull request Sep 22, 2016
* 2.8:
  Several typo fixes
  Several typo fixes
  Update console.rst
  [#6917] some tweaks after review
  [Finder] document array use for locations
xabbuh added a commit that referenced this pull request Sep 22, 2016
* 3.1:
  Several typo fixes
  Several typo fixes
  Several typo fixes
  Update console.rst
  [#6917] some tweaks after review
  [Finder] document array use for locations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants