Skip to content

Fix missing check for 'no-coverage' option - #29

Closed
Mediagone wants to merge 1 commit into
leanphp:masterfrom
Mediagone:master
Closed

Fix missing check for 'no-coverage' option#29
Mediagone wants to merge 1 commit into
leanphp:masterfrom
Mediagone:master

Conversation

@Mediagone

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!)

@ek9 ek9 Mar 21, 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.

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

Choose a reason for hiding this comment

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

Thanks!

@ek9 ek9 left a comment

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.

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

@ek9

ek9 commented Mar 21, 2018

Copy link
Copy Markdown
Member

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

ek9 commented Mar 21, 2018

Copy link
Copy Markdown
Member

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