-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Added Checkbox Console Prompt and its tests #7091
Conversation
* Zend Framework (http://framework.zend.com/) | ||
* | ||
* @link http://github.com/zendframework/zf2 for the canonical source repository | ||
* @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com) |
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.
2015 ;)
2015 :-)
2015 :-)
$this->setPromptText($promptText); | ||
} | ||
|
||
if (! count($options)) { |
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.
Can simply be if (! $options) {
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.
Should probably be handled by the setter
*/ | ||
private function setOptions($options) | ||
{ | ||
if (! is_array($options) && ! $options instanceof \Traversable) { |
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.
The console component depends on zendframework/zend-stdlib
, so you can safely use ArrayUtils::iteratorToArray()
here and let it deal with the conversion/traversal instead
Besides the review/comments: awesome! :-) |
Yeah, sorry about so many things. Just copied the Select one and made it accepting multiple options. Will rewrite it as instructed :-) |
It's fine, it's my part of the process to review everything, so those get actually caught ;-) |
Thanks! Glad to help. |
@Ocramius Guess i'm done here. Care a last review? :-) |
Just send me an email or catch me on IRC on freenode ( #zftalk ) |
/** | ||
* @var string | ||
*/ | ||
protected $promptText = 'Please select an option (Enter to finish) '; |
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.
All these can be private
in my opinion
/** | ||
* @return array | ||
*/ | ||
private function getOptions() |
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.
To be removed as well
@Ocramius Done. |
Looks good! Will pull locally and merge as soon as I can. |
Ok. I will be more careful with the code next time :-) Sorry. |
@Lansoweb this PR was actually quite clean! No need to be sorry: it's good work! Also, you can always expect some nitpicking by me :-) |
Removing unnecessary variable, fixing showResponse call
Added Checkbox Console Prompt and its tests
…elop Added Checkbox Console Prompt and its tests
Adding a new console prompt similar to the Select, but can select more than one option and returns an array with them.