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

Repeatable --coverage-src option #388

Closed
wants to merge 1 commit into from

Conversation

harmim
Copy link
Contributor

@harmim harmim commented Sep 5, 2018

  • new feature
  • BC break? yes

Added support for repeatable --coverage-src option. It is useful when you have got eg. multiple directories in root of your project which you want to generate coverage for.

@milo
Copy link
Member

@milo milo commented Sep 6, 2018

This PR looks great!
Solves #232, partially solves #225

@harmim
Copy link
Contributor Author

@harmim harmim commented Sep 8, 2018

So is it ready to be merged?

@milo
Copy link
Member

@milo milo commented Sep 9, 2018

Not yet. IMHO one thing should be done differently but I have to try it.

@@ -95,8 +95,13 @@ private function parse(): void
}

$light = $total ? $total < 5 : count(file($entry)) < 50;
$nameReplacePairs = [];
Copy link
Member

@milo milo Sep 10, 2018

Choose a reason for hiding this comment

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

There should be single replacement, common path of all sources. Otherway duplicates may appear.
And it can be done before foreach loop.

Copy link
Contributor Author

@harmim harmim Sep 10, 2018

Choose a reason for hiding this comment

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

@milo What is the easiest way to find common path of all sources?

Copy link
Member

@milo milo Sep 10, 2018

Choose a reason for hiding this comment

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

This code can be reused.

$iterator = is_dir($this->source)
? new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->source))
: new \ArrayIterator([new \SplFileInfo($this->source)]);
$iterator = new \AppendIterator();
Copy link
Member

@milo milo Sep 10, 2018

Choose a reason for hiding this comment

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

No brackets according to CS consistency.

@@ -62,12 +62,17 @@ public function __construct(string $file, string $source = null)
}
}
$source = dirname($source . 'x');
$sources[] = $source;
Copy link
Member

@milo milo Sep 10, 2018

Choose a reason for hiding this comment

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

$sources = [dirname(...)];

? new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($source))
: new \ArrayIterator([new \SplFileInfo($source)]);
$iterator->append($partialIterator);
}
Copy link
Member

@milo milo Sep 10, 2018

Choose a reason for hiding this comment

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

Since $partialIterator is not used anywhere else, write it this way:

$iterator->append(
    is_dir($source)
        ? new \Recursive.....,
        : new \Array.....
);

@milo
Copy link
Member

@milo milo commented Sep 10, 2018

Sorry if you see any mess here. I'm trying the review and resolve/unresolve conversation functionality.

@harmim harmim force-pushed the harmim-coverage-src-repetable branch from 9de88a3 to f89cab0 Compare Sep 11, 2018
@harmim
Copy link
Contributor Author

@harmim harmim commented Sep 11, 2018

@milo Thanks for your review. I have edited it.

milo pushed a commit that referenced this issue Sep 11, 2018
milo
milo approved these changes Sep 11, 2018
@milo
Copy link
Member

@milo milo commented Sep 11, 2018

Merged 2286131. Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants