-
Notifications
You must be signed in to change notification settings - Fork 39
Fix missing check for 'no-coverage' option #29
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
Conversation
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')) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this 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!
|
The fix should now be live on both |
Executing 'bin/phpspec describe ...' after installing the lib gives me the following error :
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.