Skip to content

Repeatable --coverage-src option#388

Closed
harmim wants to merge 1 commit into
nette:masterfrom
harmim:harmim-coverage-src-repetable
Closed

Repeatable --coverage-src option#388
harmim wants to merge 1 commit into
nette:masterfrom
harmim:harmim-coverage-src-repetable

Conversation

@harmim

@harmim harmim commented Sep 5, 2018

Copy link
Copy Markdown
Contributor
  • 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

milo commented Sep 6, 2018

Copy link
Copy Markdown
Member

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

@harmim

harmim commented Sep 8, 2018

Copy link
Copy Markdown
Contributor Author

So is it ready to be merged?

@milo

milo commented Sep 9, 2018

Copy link
Copy Markdown
Member

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

}

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

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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
Copy Markdown
Member

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.

}
}
$source = dirname($source . 'x');
$sources[] = $source;

Copy link
Copy Markdown
Member

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);
}

@milo milo Sep 10, 2018

Copy link
Copy Markdown
Member

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

milo commented Sep 10, 2018

Copy link
Copy Markdown
Member

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 September 11, 2018 09:15
@harmim

harmim commented Sep 11, 2018

Copy link
Copy Markdown
Contributor Author

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

milo pushed a commit that referenced this pull request Sep 11, 2018
@milo

milo commented Sep 11, 2018

Copy link
Copy Markdown
Member

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants