8000 Fix missing check for 'no-coverage' option by Mediagone · Pull Request #29 · leanphp/phpspec-code-coverage · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@Mediagone
Copy link
Contributor

Executing 'bin/phpspec describe ...' after installing the lib gives me the following error :

"The "no-coverage" option does not exist.

It might be an edge case, or config related. Unfortunately I didn't have time to investigate further into the codebase, but adding this check should prevent the error without side effect.

Let me know if you see any harm.

Executing 'bin/phpspec describe ...' gives the following error : "The 'no-coverage' option does not exist."
$skipCoverage = false;
$input = $container->get('console.input');
if ($input->getOption('no-coverage')) {
if (!$input->hasOption('no-coverage') || $input->getOption('no-coverage')) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more readable to have it check if it has an option AND if it's enabled instead:

if ($input->hasOption('no-coverage') && $input->getOption('no-coverage')) {

Functionally it is the same as the original, but I believe it is a bit easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not exactly the same, because it won't skip coverage if the option is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(in my case, I don't want coverage to be executed when creating a spec!)

Copy link
Member
@ek9 ek9 Mar 21, 2018

Choose a reason for hiding this comment

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

I understand, for this we need to add --no-coverage option to the remaining commands (currently only run has that option). I will sort this out shortly and issue a bugfix release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member
@ek9 ek9 left a comment

Choose a reason for hiding this comment

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

Hi, Thank you for contribution. Could you please make a small adjustment before I can merge it. Thanks!

@ek9
Copy link
Member
ek9 commented Mar 21, 2018

Closing this in favour of PR #31 (it has your original commit + other minor fix as noted on issue #30).

Will be merged and released shortly as bugfix releases 4.1.2 and 4.2.1 of leanphp/phpspec-code-coverage.

Thank you for contribution.

@ek9
Copy link
Member
ek9 commented Mar 21, 2018

The fix should now be live on both 4.1.2 and 4.2.1 versions (not sure which one you were using). Sorry for the trouble and do not hesitate to open an issue if there are any further problems/

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

0