Exclude some common directories by swissspidy · Pull Request #32 · wp-cli/i18n-command

swissspidy · 2018-05-30T07:59:24Z

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

See #27.

danielbachhuber · 2018-06-01T15:52:46Z

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

Should it be a CLI argument or an environment variable?

schlessera · 2018-06-05T16:11:19Z

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 · 2018-06-05T16:24:31Z

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 · 2018-06-05T16:27:05Z

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 · 2018-06-05T16:30:14Z

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 · 2018-06-19T09:59:28Z

I'm sticking with an extra argument for now.

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

schlessera · 2018-06-21T10:08:55Z

features/makepot.feature

+      """
+
+    When I run `wp i18n make-pot foo-plugin foo-plugin.pot`
+    And the foo-plugin.pot file should not contain:


Should be Then ... for readability.

schlessera · 2018-06-21T10:09:11Z

features/makepot.feature

+      """
+
+    When I run `wp i18n make-pot foo-plugin foo-plugin.pot --exclude=foo,bar`
+    And the foo-plugin.pot file should not contain:


Should be Then ... for readability.

schlessera · 2018-06-21T10:10:28Z

src/WordPressCodeExtractor.php

+			/** @var SplFileInfo $file */
 			foreach ( $files as $file ) {
-				if ( $file->isFile() && 'php' === $file->getExtension()) {
+				if (  $file->isFile() ) {


CS: double space.

schlessera · 2018-06-21T10:10:44Z

src/WordPressCodeExtractor.php


 		return $filtered_files;
 	}
+


CS: Redundant new line.

schlessera · 2018-06-21T10:16:02Z

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 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.

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.

schlessera · 2018-06-21T10:17:26Z

features/makepot.feature

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


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)

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.

schlessera

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

schlessera · 2018-06-25T11:40:53Z

features/makepot.feature

      I am being ignored
      """
+
+  Scenario: Skips excluded subfolders


For this scenario, I was thinking more along the lines of adding an exclude that contains a subfolder, like this:

wp i18n make-pot foo-plugin foo-plugin.pot --exclude=some/sub/folder

I don't think testing the other way around (omitting a subfolder that was part of parent folder being excluded) is needed, as all files within a parent folder would have the same problem then.

swissspidy · 2018-06-30T13:06:40Z

src/WordPressCodeExtractor.php


+						// Check for more complex paths, e.g. /some/sub/folder.
+						foreach( $exclude as $path_or_file ) {
+							if ( substr( $file->getPathname(), - strlen( $path_or_file ) ) === $path_or_file ) {


There's probably a better way to do this, but I couldn't think of one right now.

schlessera · 2018-07-02T07:39:36Z

src/WordPressCodeExtractor.php

+
+						// Check for more complex paths, e.g. /some/sub/folder.
+						foreach( $exclude as $path_or_file ) {
+							if ( substr( $file->getPathname(), - strlen( '/' . $path_or_file ) ) === '/' . $path_or_file ) {


You should use substr_compare instead as it is optimized towards the comparison:

$substring = '/' . $path_or_file; if ( substr_compare( $file->getPathname(), $substring, - strlen( $substring ) ) === 0 ) { }

Also, I'm wondering how safe this is when using multibyte strings. Off the top of my head, I can't think of a scenario where the comparison would fail to produce the correct result, but I'm not 100% sure. Do you think this might be an issue?

Good call.

I can't think of a scenario either right now, but you never know :-)

We could use mb_ereg in this case:

if ( false !== mb_ereg( preg_quote( '/' . $path_or_file ) . '$', $file->getPathname() ) ) { return false; }

Feels more readable anyway.

schlessera · 2018-07-02T18:34:59Z

Merged now. Good work, @swissspidy!

Exclude some common directories

Exclude some common directories

8749e24

danielbachhuber requested a review from schlessera June 1, 2018 15:52

schlessera added the command:i18n-make-pot Related to 'i18n make-pot' command label Jun 5, 2018

Add exclude argument and some tests

19f5a05

swissspidy requested a review from a team June 19, 2018 14:23

schlessera requested changes Jun 21, 2018

View reviewed changes

swissspidy added 4 commits June 25, 2018 12:16

CS fixes

c983d72

Improve wording in test

5df99ec

Remove leading and trailing slashes for file exclusion

387fa2e

Add some more test cases for file exclusion

91e8c54

schlessera requested changes Jun 25, 2018

View reviewed changes

swissspidy added 4 commits June 30, 2018 15:00

Add support and tests for more complex paths, e.g. /some/sub/folder

635dfc3

Update readme

d5379eb

Merge branch 'master' into 27-exclude-option

d4199e1

Add leading slash for path comparison

dabf336

swissspidy commented Jun 30, 2018

View reviewed changes

Fix strlen check

d2cbc67

schlessera requested changes Jul 2, 2018

View reviewed changes

Use multi-byte safe regular expression to check string ending

b857029

schlessera approved these changes Jul 2, 2018

View reviewed changes

schlessera merged commit f56e9eb into master Jul 2, 2018

schlessera deleted the 27-exclude-option branch July 2, 2018 18:34

schlessera added this to the 0.1.0 milestone Jul 2, 2018

schlessera added a commit that referenced this pull request Jan 6, 2022

Merge pull request #32 from wp-cli/27-exclude-option

03e8590

Exclude some common directories

Conversation

swissspidy commented May 30, 2018

Uh oh!

danielbachhuber commented Jun 1, 2018

Uh oh!

schlessera commented Jun 5, 2018

Uh oh!

swissspidy commented Jun 5, 2018

Uh oh!

danielbachhuber commented Jun 5, 2018

Uh oh!

schlessera commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera commented Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

schlessera commented Jun 5, 2018 •

edited

Loading

schlessera left a comment •

edited

Loading