Added Checkbox Console Prompt and its tests #7091

Merged
merged 10 commits into from Feb 24, 2015

Conversation

Projects
None yet
4 participants
@Lansoweb
Contributor

Lansoweb commented Jan 1, 2015

Adding a new console prompt similar to the Select, but can select more than one option and returns an array with them.

library/Zend/Console/Prompt/Checkbox.php
+ * 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)

This comment has been minimized.

@samsonasik

samsonasik Jan 1, 2015

Contributor

2015 ;)

@samsonasik

samsonasik Jan 1, 2015

Contributor

2015 ;)

+ * 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)

This comment has been minimized.

@samsonasik

samsonasik Jan 1, 2015

Contributor

2015 ;)

@samsonasik

samsonasik Jan 1, 2015

Contributor

2015 ;)

This comment has been minimized.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Yes, thanks and Happy New Year! :-)

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Yes, thanks and Happy New Year! :-)

library/Zend/Console/Prompt/Checkbox.php
+ $this->setPromptText($promptText);
+ }
+
+ if (! count($options)) {

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Can simply be if (! $options) {

@Ocramius

Ocramius Jan 1, 2015

Member

Can simply be if (! $options) {

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Should probably be handled by the setter

@Ocramius

Ocramius Jan 1, 2015

Member

Should probably be handled by the setter

library/Zend/Console/Prompt/Checkbox.php
+ * @param array|\Traversable $options
+ * @throws Exception\BadMethodCallException
+ */
+ public function setOptions($options)

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Can you make this method private?

@Ocramius

Ocramius Jan 1, 2015

Member

Can you make this method private?

This comment has been minimized.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Sure. Copied from Select prompt. Should i change it there too?

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Sure. Copied from Select prompt. Should i change it there too?

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

No, I simply want to avoid unnecessary public API for newly introduced code

@Ocramius

Ocramius Jan 1, 2015

Member

No, I simply want to avoid unnecessary public API for newly introduced code

This comment has been minimized.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Ok.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Ok.

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * @return array
+ */
+ public function getOptions()

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Same as above: private

@Ocramius

Ocramius Jan 1, 2015

Member

Same as above: private

+ */
+ public function __construct($autoRewind = true)
+ {
+ $this->autoRewind = $autoRewind;

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Cast to (bool)

@Ocramius

Ocramius Jan 1, 2015

Member

Cast to (bool)

library/Zend/Console/Prompt/Checkbox.php
+ $this->echo = $oldEcho;
+
+ // Display selected option if echo is enabled
+ if ($this->echo) {

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

This method is too long/complex: split it into multiple private methods

@Ocramius

Ocramius Jan 1, 2015

Member

This method is too long/complex: split it into multiple private methods

This comment has been minimized.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

No problem. Copied from the Select Prompt too. Will made the changes. Thanks!

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

No problem. Copied from the Select Prompt too. Will made the changes. Thanks!

@Ocramius Ocramius self-assigned this Jan 1, 2015

@Ocramius Ocramius added this to the 2.4.0 milestone Jan 1, 2015

library/Zend/Console/Prompt/Checkbox.php
+ */
+ private function setOptions($options)
+ {
+ if (! is_array($options) && ! $options instanceof \Traversable) {

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

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

@Ocramius

Ocramius Jan 1, 2015

Member

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

library/Zend/Console/Prompt/Checkbox.php
+ if (! is_array($options)) {
+ $this->options = array();
+ foreach ($options as $k => $v) {
+ $this->options[$k] = $v;

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Are values supposed to always be string? If so, then an explicit cast would be necessary here

@Ocramius

Ocramius Jan 1, 2015

Member

Are values supposed to always be string? If so, then an explicit cast would be necessary here

library/Zend/Console/Prompt/Checkbox.php
+ private function setOptions($options)
+ {
+ if (! is_array($options) && ! $options instanceof \Traversable) {
+ throw new Exception\BadMethodCallException('Please specify an array or Traversable object as options');

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

This should probably be an InvalidArgumentException, not a BadMethodCallException

@Ocramius

Ocramius Jan 1, 2015

Member

This should probably be an InvalidArgumentException, not a BadMethodCallException

library/Zend/Console/Prompt/Checkbox.php
+
+use Zend\Console\Exception;
+
+class Checkbox extends Char

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Do you actually need to inherit from Char? Seems like it is not really a subtype (from a logical perspective)

@Ocramius

Ocramius Jan 1, 2015

Member

Do you actually need to inherit from Char? Seems like it is not really a subtype (from a logical perspective)

library/Zend/Console/Prompt/Checkbox.php
+ $this->setAllowEmpty($allowEmpty);
+ }
+
+ if ($echo !== null) {

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Can probably skip this and always pass $echo to the setter

@Ocramius

Ocramius Jan 1, 2015

Member

Can probably skip this and always pass $echo to the setter

library/Zend/Console/Prompt/Checkbox.php
+
+ $this->setOptions($options);
+
+ if ($allowEmpty !== null) {

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Can probably skip this and always pass $allowEmpty to the setter

@Ocramius

Ocramius Jan 1, 2015

Member

Can probably skip this and always pass $allowEmpty to the setter

library/Zend/Console/Prompt/Checkbox.php
+ * Ask the user to select any number of pre-defined options
+ *
+ * @param string $promptText The prompt text to display in console
+ * @param array $options Allowed options

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Must document that it may be a Traversable

@Ocramius

Ocramius Jan 1, 2015

Member

Must document that it may be a Traversable

library/Zend/Console/Prompt/Checkbox.php
+ protected $ignoreCase = true;
+
+ /**
+ * @var array

This comment has been minimized.

@Ocramius

Ocramius Jan 1, 2015

Member

Are these mixed values? Or string[]?

@Ocramius

Ocramius Jan 1, 2015

Member

Are these mixed values? Or string[]?

This comment has been minimized.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Mixed at first, but i guess only string (an maybe numeric) makes sense here.

@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Mixed at first, but i guess only string (an maybe numeric) makes sense here.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 1, 2015

Member

Besides the review/comments: awesome! :-)

Member

Ocramius commented Jan 1, 2015

Besides the review/comments: awesome! :-)

@Lansoweb

This comment has been minimized.

Show comment
Hide comment
@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Yeah, sorry about so many things. Just copied the Select one and made it accepting multiple options. Will rewrite it as instructed :-)

Contributor

Lansoweb commented Jan 1, 2015

Yeah, sorry about so many things. Just copied the Select one and made it accepting multiple options. Will rewrite it as instructed :-)

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 1, 2015

Member

sorry about so many things

It's fine, it's my part of the process to review everything, so those get actually caught ;-)
The implementation just needed some cleanups, that's it. Poke me again if you need another round of reviews.

Member

Ocramius commented Jan 1, 2015

sorry about so many things

It's fine, it's my part of the process to review everything, so those get actually caught ;-)
The implementation just needed some cleanups, that's it. Poke me again if you need another round of reviews.

@Lansoweb

This comment has been minimized.

Show comment
Hide comment
@Lansoweb

Lansoweb Jan 1, 2015

Contributor

Besides the review/comments: awesome! :-)

Thanks! Glad to help.

Contributor

Lansoweb commented Jan 1, 2015

Besides the review/comments: awesome! :-)

Thanks! Glad to help.

@Lansoweb

This comment has been minimized.

Show comment
Hide comment
@Lansoweb

Lansoweb Jan 1, 2015

Contributor

@Ocramius Guess i'm done here. Care a last review? :-)
What's the best way to contact you? Want to talk about contribution.

Contributor

Lansoweb commented Jan 1, 2015

@Ocramius Guess i'm done here. Care a last review? :-)
What's the best way to contact you? Want to talk about contribution.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 2, 2015

Member

What's the best way to contact you?

Just send me an email or catch me on IRC on freenode ( #zftalk )

Member

Ocramius commented Jan 2, 2015

What's the best way to contact you?

Just send me an email or catch me on IRC on freenode ( #zftalk )

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * @var string
+ */
+ protected $promptText = 'Please select an option (Enter to finish) ';

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

All these can be private in my opinion

@Ocramius

Ocramius Jan 2, 2015

Member

All these can be private in my opinion

library/Zend/Console/Prompt/Checkbox.php
+
+class Checkbox extends AbstractPrompt
+{
+

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Remove this newline

@Ocramius

Ocramius Jan 2, 2015

Member

Remove this newline

library/Zend/Console/Prompt/Checkbox.php
+use Zend\Console\Exception;
+use Zend\Stdlib\ArrayUtils;
+
+class Checkbox extends AbstractPrompt

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

As per discussion in #6962, please make this class final

@Ocramius

Ocramius Jan 2, 2015

Member

As per discussion in #6962, please make this class final

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * @return string
+ */
+ private function getPromptText()

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Not needed (the advantages of private ;-) )

@Ocramius

Ocramius Jan 2, 2015

Member

Not needed (the advantages of private ;-) )

library/Zend/Console/Prompt/Checkbox.php
+ * @param string $promptText
+ * @return \Zend\Console\Prompt\Checkbox
+ */
+ private function setPromptText($promptText)

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Not needed (the advantages of private ;-) )

@Ocramius

Ocramius Jan 2, 2015

Member

Not needed (the advantages of private ;-) )

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * @return boolean
+ */
+ private function getIgnoreCase()

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Not needed (the advantages of private ;-) )

@Ocramius

Ocramius Jan 2, 2015

Member

Not needed (the advantages of private ;-) )

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Also subsequent setters/getters

@Ocramius

Ocramius Jan 2, 2015

Member

Also subsequent setters/getters

This comment has been minimized.

@Lansoweb

Lansoweb Jan 2, 2015

Contributor

@Ocramius Wait, forgot to remove the setter

@Lansoweb

Lansoweb Jan 2, 2015

Contributor

@Ocramius Wait, forgot to remove the setter

This comment has been minimized.

@Lansoweb

Lansoweb Jan 2, 2015

Contributor

@Ocramius Ok, tests ok now.

@Lansoweb

Lansoweb Jan 2, 2015

Contributor

@Ocramius Ok, tests ok now.

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * @return array
+ */
+ private function getOptions()

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

To be removed as well

@Ocramius

Ocramius Jan 2, 2015

Member

To be removed as well

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * Read char from console
+ */
+ $char = $this->getConsole()->readChar($mask);

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Return directly

@Ocramius

Ocramius Jan 2, 2015

Member

Return directly

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * Normalize the mask if case is irrelevant
+ */
+ if ($this->ignoreCase) {

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Return early instead

@Ocramius

Ocramius Jan 2, 2015

Member

Return early instead

library/Zend/Console/Prompt/Checkbox.php
+ * Normalize the mask if case is irrelevant
+ */
+ if ($this->ignoreCase) {
+ $mask = strtolower($mask); // lowercase all

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Can you inline all these instead of overwriting $mask?

@Ocramius

Ocramius Jan 2, 2015

Member

Can you inline all these instead of overwriting $mask?

library/Zend/Console/Prompt/Checkbox.php
+ private function prepareMask()
+ {
+ $mask = implode("", array_keys($this->options));
+ $mask .= "\r\n";

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Inline with previous assignment

@Ocramius

Ocramius Jan 2, 2015

Member

Inline with previous assignment

library/Zend/Console/Prompt/Checkbox.php
+ /**
+ * @var string
+ */
+ private $promptText = 'Please select an option (Enter to finish) ';

This comment has been minimized.

@Ocramius

Ocramius Jan 2, 2015

Member

Default values are also not needed anymore if __construct deals with all of them ;-)

@Ocramius

Ocramius Jan 2, 2015

Member

Default values are also not needed anymore if __construct deals with all of them ;-)

@Lansoweb

This comment has been minimized.

Show comment
Hide comment
@Lansoweb

Lansoweb Jan 2, 2015

Contributor

@Ocramius Done.

Contributor

Lansoweb commented Jan 2, 2015

@Ocramius Done.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 2, 2015

Member

Looks good! Will pull locally and merge as soon as I can.

Member

Ocramius commented Jan 2, 2015

Looks good! Will pull locally and merge as soon as I can.

@Lansoweb

This comment has been minimized.

Show comment
Hide comment
@Lansoweb

Lansoweb Jan 2, 2015

Contributor

Ok. I will be more careful with the code next time :-) Sorry.

Contributor

Lansoweb commented Jan 2, 2015

Ok. I will be more careful with the code next time :-) Sorry.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jan 2, 2015

Member

@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 :-)

Member

Ocramius commented Jan 2, 2015

@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 :-)

Code cleanup
Removing unnecessary variable, fixing showResponse call

@weierophinney weierophinney merged commit cbe91b2 into zendframework:develop Feb 24, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

weierophinney added a commit that referenced this pull request Feb 24, 2015

Merge pull request #7091 from Lansoweb/develop
Added Checkbox Console Prompt and its tests

weierophinney added a commit that referenced this pull request Feb 24, 2015

weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#7091 from Lansoweb/dev…
…elop

Added Checkbox Console Prompt and its tests

weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment