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

Exclude some common directories #32

Merged
merged 12 commits into from Jul 2, 2018

Conversation

3 participants
@swissspidy
Collaborator

swissspidy commented May 30, 2018

Does not yet contain a CLI argument to extend/override exclusions.

See #27.

@danielbachhuber danielbachhuber requested a review from schlessera Jun 1, 2018

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jun 1, 2018

Member

Does not yet contain a CLI argument to extend/override exclusions.

Should it be a CLI argument or an environment variable?

Member

danielbachhuber commented Jun 1, 2018

Does not yet contain a CLI argument to extend/override exclusions.

Should it be a CLI argument or an environment variable?

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Jun 5, 2018

Member

As this is meant to extract code from a WordPress plugin or theme, I wonder whether it wouldn't be preferable to use a WordPress filter to make the exclusions customizable from within a plugin/theme...

Member

schlessera commented Jun 5, 2018

As this is meant to extract code from a WordPress plugin or theme, I wonder whether it wouldn't be preferable to use a WordPress filter to make the exclusions customizable from within a plugin/theme...

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Jun 5, 2018

Collaborator

As this is meant to extract code from a WordPress plugin or theme, I wonder whether it wouldn't be preferable to use a WordPress filter to make the exclusions customizable from within a plugin/theme...

wp i18n make-pot is something that's being run during a build step. There's no WordPress involved there.

Should it be a CLI argument or an environment variable?

Good question!

An env var might be easier when dealing with multiple projects. That way you don't have to add the same exclusions over and over again.

I also wonder whether this option should just extend default exclusions like node_modules or override them. I tend to think the former makes more sense.

Collaborator

swissspidy commented Jun 5, 2018

As this is meant to extract code from a WordPress plugin or theme, I wonder whether it wouldn't be preferable to use a WordPress filter to make the exclusions customizable from within a plugin/theme...

wp i18n make-pot is something that's being run during a build step. There's no WordPress involved there.

Should it be a CLI argument or an environment variable?

Good question!

An env var might be easier when dealing with multiple projects. That way you don't have to add the same exclusions over and over again.

I also wonder whether this option should just extend default exclusions like node_modules or override them. I tend to think the former makes more sense.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jun 5, 2018

Member

Here's some prior art: https://github.com/wp-cli/find-command#using

[--include_ignored_paths=<paths>]
	Include additional ignored paths as CSV (e.g. '/sys-backup/,/temp/').
Member

danielbachhuber commented Jun 5, 2018

Here's some prior art: https://github.com/wp-cli/find-command#using

[--include_ignored_paths=<paths>]
	Include additional ignored paths as CSV (e.g. '/sys-backup/,/temp/').
@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Jun 5, 2018

Member

wp i18n make-pot is something that's being run during a build step. There's no WordPress involved there.

Ah, right, it uses @when before_wp_load.

I'm not sure using environment variables is useful in this case. A parameter would allow for immediate configuration, and you can still configure it per-user or per-site through the wp-cli.yml file.

Member

schlessera commented Jun 5, 2018

wp i18n make-pot is something that's being run during a build step. There's no WordPress involved there.

Ah, right, it uses @when before_wp_load.

I'm not sure using environment variables is useful in this case. A parameter would allow for immediate configuration, and you can still configure it per-user or per-site through the wp-cli.yml file.

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Jun 19, 2018

Collaborator

I'm sticking with an extra argument for now.

Wondering though if I should rename exclude to ignore 🤷‍♂️

Collaborator

swissspidy commented Jun 19, 2018

I'm sticking with an extra argument for now.

Wondering though if I should rename exclude to ignore 🤷‍♂️

@swissspidy swissspidy requested a review from wp-cli/committers Jun 19, 2018

Show outdated Hide outdated features/makepot.feature
Show outdated Hide outdated features/makepot.feature
Show outdated Hide outdated src/WordPressCodeExtractor.php
Show outdated Hide outdated src/WordPressCodeExtractor.php
new RecursiveDirectoryIterator( $dir, RecursiveDirectoryIterator::SKIP_DOTS ),
function ( $file, $key, $iterator ) use ( $exclude ) {
/** @var SplFileInfo $file */
if ( in_array( $file->getBasename(), $exclude, true ) ) {

This comment has been minimized.

@schlessera

schlessera Jun 21, 2018

Member

This will fail if a path is returned with a trailing slash. Using trailing slashes is often done for avoiding partial matches. This either needs specific handling of trailing slashes, or an explicit mention in the argument documentation.

@schlessera

schlessera Jun 21, 2018

Member

This will fail if a path is returned with a trailing slash. Using trailing slashes is often done for avoiding partial matches. This either needs specific handling of trailing slashes, or an explicit mention in the argument documentation.

This comment has been minimized.

@swissspidy

swissspidy Jun 25, 2018

Collaborator

I don't think getBasename() ever returns a path with a trailing slash. The values in $exclude could have indeed have trailing slashes, yes.

I guess I can safely add a change to remove any slashes at the beginning and the end of the excluded directories/files and document this.

@swissspidy

swissspidy Jun 25, 2018

Collaborator

I don't think getBasename() ever returns a path with a trailing slash. The values in $exclude could have indeed have trailing slashes, yes.

I guess I can safely add a change to remove any slashes at the beginning and the end of the excluded directories/files and document this.

I am being ignored
"""
Scenario: Skips additionally excluded folders

This comment has been minimized.

@schlessera

schlessera Jun 21, 2018

Member

Tests should cover these as well to make sure they work (and continue to do so):

  • subfolder (deeper than in immediate root)
  • file
  • file in subfolder (deeper than in immediate root)
  • partial match (NOT being excluded)
@schlessera

schlessera Jun 21, 2018

Member

Tests should cover these as well to make sure they work (and continue to do so):

  • subfolder (deeper than in immediate root)
  • file
  • file in subfolder (deeper than in immediate root)
  • partial match (NOT being excluded)

This comment has been minimized.

@swissspidy

swissspidy Jun 25, 2018

Collaborator

Agreed. Will add these in a spare moment.

Also, I was just wondering whether glob-like patterns should be supported using something like fnmatch, but that's probably something for a future iteration.

@swissspidy

swissspidy Jun 25, 2018

Collaborator

Agreed. Will add these in a spare moment.

Also, I was just wondering whether glob-like patterns should be supported using something like fnmatch, but that's probably something for a future iteration.

swissspidy added some commits Jun 25, 2018

@schlessera

schlessera requested changes Jun 25, 2018 edited

Looks good apart from the 1 test case where I think we had different interpretations.

Show outdated Hide outdated features/makepot.feature

swissspidy added some commits Jun 30, 2018

@schlessera schlessera merged commit f56e9eb into master Jul 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera deleted the 27-exclude-option branch Jul 2, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Jul 2, 2018

Member

Merged now. Good work, @swissspidy!

Member

schlessera commented Jul 2, 2018

Merged now. Good work, @swissspidy!

@schlessera schlessera added this to the 0.1.0 milestone Jul 2, 2018

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