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

Determine includes and excludes based on scores #104

Merged
merged 8 commits into from Oct 31, 2018

Conversation

2 participants
@herregroen
Copy link
Contributor

herregroen commented Oct 30, 2018

Include and exclude matches are based on the strength of the match with:

  1. path matches
  2. filename matches
  3. regex matches

Can probably use improvement but this should cover the base scenarios.

herregroen added some commits Oct 30, 2018

@@ -70,18 +75,18 @@ public function test_can_exclude_override_wildcard() {
}
public function test_can_exclude_override_matching_directory() {
$result = IterableCodeExtractor::getFilesFromDirectory( self::$base, [ 'foo/bar/*' ], ['excluded'], [ 'php' ] );
$result = IterableCodeExtractor::getFilesFromDirectory( self::$base, [ 'foo/bar/*' ], ['foo/bar/excluded/*'], [ 'php' ] );

This comment has been minimized.

@schlessera

schlessera Oct 30, 2018

Member

CS: spaces in array notation are not consistent.

@@ -172,16 +159,16 @@ function ( $file, $key, $iterator ) use ( $include, $exclude, $extensions ) {
/** @var RecursiveCallbackFilterIterator $iterator */
/** @var SplFileInfo $file */
if ( static::isExcluded( $file, $exclude ) && ( empty( $include ) || ! static::isIncluded( $file, $include ) ) ) {
return false;
if ( $iterator->hasChildren() ) {

This comment has been minimized.

@schlessera

schlessera Oct 30, 2018

Member

We might need to make this smarter, to avoid traversing the entire node_modules for example, when there's no rule that "includes" anything related to that.

function ( $component) { return $component !== '*'; }
)
);
if ( $base_score === 0 ) {

This comment has been minimized.

@schlessera

schlessera Oct 30, 2018

Member

CS: Yoda here, as we use it everywhere else as well.

herregroen added some commits Oct 31, 2018

$pattern = preg_quote( str_replace( '*', '__wildcard__', $path_or_file ) );
$pattern = '/' . str_replace( '__wildcard__', '(.+)', $pattern ) . '$';
foreach ( $matchers as $path_or_file ) {
if (

This comment has been minimized.

@schlessera

schlessera Oct 31, 2018

Member

I'd like to have a short comment here describing what is being checked. Maybe something like this:

Suggested change Beta
if (
// Without a wildcard, we can directly check the folder hierarchy.
if (
$root_relative_path = str_replace( static::$dir, '', $file->getPathname() );
if ( false !== mb_ereg( $pattern, $root_relative_path ) ) {
if (

This comment has been minimized.

@schlessera

schlessera Oct 31, 2018

Member

This needs a comment as well. I'm not 100% sure what the reason is for checking both ways, I assume this is done for being overly conservative. This certainly needs an explanation.

This comment has been minimized.

@herregroen

herregroen Oct 31, 2018

Contributor

Needed for partial wildcards like this:

Given an include wildcard of: dir/wp-* any directory starting with dir/wp- should always be checked. We're checking if the directory starts with the matcher.

Whereas for normal wildcards the check is the other way around: one/two/three/*.js means that a directory called one should be checked to see if the matcher starts with the directory.

@schlessera schlessera added this to the 2.0.3 milestone Oct 31, 2018

@schlessera schlessera merged commit 4c12776 into wp-cli:master Oct 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment