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] Ignore paths from .gitignore #26714 #30448

Merged
merged 1 commit into from Mar 22, 2019

Conversation

Projects
None yet
@amaabdou
Copy link
Contributor

commented Mar 5, 2019

Q A
Branch? master for features
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26714
License MIT
Doc PR symfony/symfony-docs#...

Implementation of feature request #26714

Finder::ignoreVCS() is great at ignoring file patterns for the files created by popular VCS systems.

However, it would be great to be able to instruct Finder to actually exclude the paths excluded by .gitignore.

So if we have .gitignore:

vendor/
cache/

Finder::create()
->files()
->ignoreVCS(true) // <--- Ignores .git
->ignoreVCSIgnored(true); // <--- Ignores vendor/ and cache/

@amaabdou amaabdou changed the title issue #26714 v1 [Finder] Ignore paths from .gitignore #26714 Mar 5, 2019

@amaabdou amaabdou force-pushed the amaabdou:feature/issue_26714 branch from 54c6b04 to 2bfe1de Mar 5, 2019

Show resolved Hide resolved src/Symfony/Component/Finder/Finder.php Outdated
Show resolved Hide resolved src/Symfony/Component/Finder/Finder.php Outdated

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 7, 2019

@dimabory

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@amaabdou could you please take a look at this https://labs.consol.de/development/git/2017/02/22/gitignore.html and add more test with advanced use cases?

@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I added as many cases I could,If I missed anything or more cases need or something is unclear
Or more reviews needed
please add a comment

@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

also I could not change PR from Draft to ready to be reviewed, I do not see the "Ready to review" button appeared today not sure how :D

@amaabdou amaabdou marked this pull request as ready for review Mar 12, 2019

@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

is there anything I can do to move this forward ?

@OskarStark
Copy link
Contributor

left a comment

Great job 🔥

Show resolved Hide resolved src/Symfony/Component/Finder/Finder.php Outdated
Show resolved Hide resolved src/Symfony/Component/Finder/Finder.php Outdated
Show resolved Hide resolved src/Symfony/Component/Finder/Finder.php Outdated
@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

is there anything else I can do to move this forward ?

@fabpot

fabpot approved these changes Mar 22, 2019

Copy link
Member

left a comment

Can you add a note in the component CHANGELOG file?

@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@fabpot just did , is this one okay ?

@fabpot fabpot force-pushed the amaabdou:feature/issue_26714 branch from f7a664e to 9491393 Mar 22, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Great work @amaabdou, thank you

@fabpot fabpot merged commit 9491393 into symfony:master Mar 22, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 22, 2019

feature #30448 [Finder] Ignore paths from .gitignore #26714 (amaabdou)
This PR was squashed before being merged into the 4.3-dev branch (closes #30448).

Discussion
----------

 [Finder] Ignore paths from .gitignore #26714

| Q             | A
| ------------- | ---
| Branch?       | master for features
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26714
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->
Implementation of feature request #26714

Finder::ignoreVCS() is great at ignoring file patterns for the files created by popular VCS systems.

However, it would be great to be able to instruct Finder to actually exclude the paths excluded by .gitignore.

So if we have .gitignore:

vendor/
cache/

Finder::create()
	->files()
    ->ignoreVCS(true) // <--- Ignores `.git`
	->ignoreVCSIgnored(true); // <--- Ignores vendor/ and cache/

Commits
-------

9491393  [Finder] Ignore paths from .gitignore #26714
@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Thank you for this great new feature @amaabdou, would you please create a documentation PR in the symfony-docs repository?

@SpacePossum

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

thanks @amaabdou :)

@amaabdou amaabdou deleted the amaabdou:feature/issue_26714 branch Mar 22, 2019

@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@OskarStark Yep yep, of course, working on this

@hkdobrev

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Thank you @amaabdou! ❤️

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 26, 2019

minor #11224 Adding ignoreVCSIgnored documentation (amaabdou)
This PR was merged into the master branch.

Discussion
----------

Adding ignoreVCSIgnored documentation

<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/roadmap for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `master` for features of unreleased versions).

-->
Adding documentation for symfony/symfony#30448

Commits
-------

6d7ec3f adding ignoreVCSIgnored documentation
@stof

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

ignoreVCS handles all VCS (well, at least all the ones we know about).
This new ignoreVCSIgnored seems to be actually ignoreGitIgnored as it only cares about .gitignore.

I think we should either add support for other VCS or rename method.

And this method is also broken, as it looks for a .gitignore file in the directory passed to in(). But that file may be present in a parent folder (a Symfony project often has all gitignore rules defined at the project root, even when ignoring nested files/folders) or in subdirectories (with rules applying only in the given subdirectory).

@amaabdou

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@stof You are right, I've created two issues

one for the parent directory search, #30739.
another one for adding support for mercurial #30738, not sure what other version control systems also to suggest to add

Hopefully will work on them on weekend if no one else worked on them

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.