Add new method `promptByMultipleKeys()` in CLI class #6302

rivalarya · 2022-07-27T09:10:42Z

Description
I make this method because method promptByKey() just can read one value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

- make a test for the method - write the method in user guide

- fix title urderline in user guide

system/CLI/CLI.php

paulbalandan

If this is the same with promptByKey() except for support for multiple keys. Please extract the common code in them instead of duplicating. This will make maintenance easier.

My initial comments below:

paulbalandan · 2022-07-29T06:56:57Z

system/CLI/CLI.php

+        if (is_string($text)) {
+            $text = [$text];
+        } elseif (! is_array($text)) {
+            throw new InvalidArgumentException('$text can only be string');
+        }


This part is confusing.

The PHPDoc says $text is a string.

The method parameter says it is untyped.

The conditional says $text must be a string|array

The throwable says $text can only be a string

paulbalandan · 2022-07-29T06:58:12Z

system/CLI/CLI.php

+            throw new InvalidArgumentException('$text can only be string');
+        }
+
+        if (is_array($options) && $options) {


is_array check is always true because of the type declaration above

paulbalandan · 2022-07-29T06:59:19Z

system/CLI/CLI.php

+        } else {
+            throw new InvalidArgumentException('$options can only be array');
+        }


Based on the previous if, if $options is an array but empty, the throwable will say $options can only be an array which is misleading.

paulbalandan · 2022-07-29T07:04:32Z

system/CLI/CLI.php

+            CLI::write(CLI::color($name, 'green') . CLI::wrap($description, 125, $keyMaxLength + 4));
+        }
+        static::fwrite(STDOUT, (trim((string) ((int) $text)) ? ' ' : '') . $extraOutput . ': ');
+        $input = trim(static::input()) ?: $default;


This will hang for 6 hours in the GH actions because the unit test is waiting for input. If you can in a way extract the non-testable part(s) in another method so that it can be mocked in tests.

Now we cannot write test for it. See #6076
It is a problem.

Hmm. This is the problem with using static methods. It is not mockable by PHPUnit.

@paulbalandan It seems difficult to ask the contributor to add test code.
Do we accept this PR without test code?

Ah yes. What I did before in promptByKey PR is to check locally the changes then suggest changes, if any, since prompt is not testable.

- delete unit test for promptByMultipleKeys because codeigniter4#6076 - change method name in docs and changelog

user_guide_src/source/cli/cli_library.rst

kenjis · 2022-08-05T07:47:52Z

Sorry, this branch has conflicts. Can you rebase and solve the conflicts?

And now we can write test for CLI::input(). See #6335
Could you try to add a test if possible?

rivalarya · 2022-08-05T08:17:34Z

Sorry, this branch has conflicts. Can you rebase and solve the conflicts?

And now we can write test for CLI::input(). See #6335 Could you try to add a test if possible?

okay. i will write the test

…to add-method-promptByMultipleKey-in-CLI

system/CLI/CLI.php

kenjis · 2022-08-10T02:25:34Z

promptByKey():

$ php public/index.php 

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

[0, 1, 2]: x
The temp field must be one of: 0, 1, 2.

[0, 1, 2]:

promptByMultipleKeys():

$ php public/index.php 


Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton
 [0, 1, 2]
x
Please select correctly

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton
 [0, 1, 2]

kenjis · 2022-08-10T02:33:01Z

The output format is a bit different from promptByKey().

Can you make the same?
I did not know how to input multiple values.

For example:

$ php public/index.php 

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2]: x
Please select correctly.

You can specify multiple values separated by commas.
[0, 1, 2]:

system/CLI/CLI.php

user_guide_src/source/cli/cli_library/023.php

system/CLI/CLI.php

kenjis

See my comments.

rivalarya · 2022-08-10T02:53:51Z

The output format is a bit different from promptByKey().

Can you make the same?

I did not know how to input multiple values.

For example:
$ php public/index.php 

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2]: x
Please select correctly.

You can specify multiple values separated by commas.
[0, 1, 2]:

So, the output should only return the value of the selected key?
There is an example in the documentation. see here

kenjis · 2022-08-10T03:09:18Z

So, the output should only return the value of the selected key?

I think key value pair is okay.

There is an example in the documentation.

End users don't know whether promptByKey() or promptByMultipleKeys() is used.

rivalarya · 2022-08-10T03:36:39Z

Please select correctly.

You can specify multiple values separated by commas.
[0, 1, 2]:

In promptByKey, when a user fails in validation input, the return is:

The temp field must be one of: 0, 1, 2, 3.

[0, 1, 2, 3]:

But in promptByMultipleKeys, when a user fails in validation input, the return is:

Please select correctly.

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

[0, 1, 2]:

Is it fine? or should I replace the return in promptByMultipleKeys?

kenjis · 2022-08-10T04:20:47Z

My opinion is the following:

$ php public/index.php 

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2]: x
Please select correctly.

You can specify multiple values separated by commas.
[0, 1, 2]:

kenjis · 2022-08-14T07:19:44Z

It seems the input validation is not correct.

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2] : ^,2

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ $hobbies                                                                                                                     │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (2) [
    0 => string (12) "Playing game"
    2 => string (9) "Badminton"
]
════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Called from .../app/Controllers/Home.php:13 [dd()]

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2] : ,

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ $hobbies                                                                                                                     │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (1) [
    0 => string (12) "Playing game"
]
════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Called from .../app/Controllers/Home.php:13 [dd()]

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2] : |^

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ $hobbies                                                                                                                     │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (1) [
    0 => string (12) "Playing game"
]
════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Called from .../app/Controllers/Home.php:13 [dd()]

kenjis

Please fix the input validation.

kenjis · 2022-08-18T02:19:29Z

You have two while (true). It seems they have the different validation rules.
They should be combined into one.

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2] : 1,2,5
Please select correctly.

You can specify multiple values separated by commas.
[0, 1, 2] : 2.5

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ $hobbies                                                                                                                         │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (1) [
    2 => string (9) "Badminton"
]
════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Called from .../app/Controllers/Home.php:13 [dd()]

Select your hobbies:
  [0]  Playing game
  [1]  Sleep
  [2]  Badminton

You can specify multiple values separated by commas.
[0, 1, 2] : 1,2,5
Please select correctly.

You can specify multiple values separated by commas.
[0, 1, 2] : ^

┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ $hobbies                                                                                                                         │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (1) [
    0 => string (12) "Playing game"
]
════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Called from .../app/Controllers/Home.php:13 [dd()]

- combine two validation to one while loop

system/CLI/CLI.php

user_guide_src/source/cli/cli_library/023.php

kenjis · 2022-08-18T07:30:52Z

@rifalarya-2 I put two inline comments. That's all.

Co-authored-by: kenjis <[email protected]>

rivalarya · 2022-08-18T08:15:35Z

@rifalarya-2 I put two inline comments. That's all.

Thank you

kenjis

LGTM! Thank you.

- make method promptByMultipleKey() in class CLI

bbf9daa

- make a test for the method - write the method in user guide

kenjis added 4.3 enhancement labels Jul 27, 2022

rivalarya added 4 commits July 27, 2022 17:45

- add the method in docs upgrading v4.3.0

f7ffecc

- fix title urderline in user guide

run CS-fix

4583497

resolve grammar(?)

dbf3962

change method name in unit test

60f878d

kenjis changed the title ~~Add new method in CLI class~~ Add new method promptByKey() in CLI class Jul 28, 2022

kenjis changed the title ~~Add new method promptByKey() in CLI class~~ Add new method promptByMultipleKey() in CLI class Jul 28, 2022

kenjis reviewed Jul 28, 2022

View reviewed changes

system/CLI/CLI.php Outdated Show resolved Hide resolved

kenjis reviewed Jul 28, 2022

View reviewed changes

system/CLI/CLI.php Outdated Show resolved Hide resolved

rivalarya changed the title ~~Add new method promptByMultipleKey() in CLI class~~ Add new method promptByMultipleKeys() in CLI class Jul 29, 2022

paulbalandan reviewed Jul 29, 2022

View reviewed changes

- refactor code

c07de8e

- delete unit test for promptByMultipleKeys because codeigniter4#6076 - change method name in docs and changelog

kenjis reviewed Jul 31, 2022

View reviewed changes

user_guide_src/source/cli/cli_library.rst Outdated Show resolved Hide resolved

style guide fix

fc70bfa

kenjis added the tests needed label Aug 3, 2022

kenjis added the stale label Aug 5, 2022

rivalarya added 2 commits August 5, 2022 15:34

Merge branch '4.3' of https://github.com/codeigniter4/CodeIgniter4 in…

edb35b7

…to add-method-promptByMultipleKey-in-CLI

add unit test

d886c9d

rivalarya force-pushed the add-method-promptByMultipleKey-in-CLI branch from 687e80f to d886c9d Compare August 5, 2022 09:46

kenjis removed stale tests needed labels Aug 5, 2022

kenjis reviewed Aug 10, 2022

View reviewed changes

system/CLI/CLI.php Outdated Show resolved Hide resolved

kenjis reviewed Aug 10, 2022

View reviewed changes

system/CLI/CLI.php Outdated Show resolved Hide resolved

kenjis reviewed Aug 10, 2022

View reviewed changes

user_guide_src/source/cli/cli_library/023.php Outdated Show resolved Hide resolved

kenjis reviewed Aug 10, 2022

View reviewed changes

system/CLI/CLI.php Outdated Show resolved Hide resolved

kenjis requested changes Aug 10, 2022

View reviewed changes

rivalarya added 3 commits August 10, 2022 15:31

Refactor the method

761e5d0

Fix docs about the method

f48ae74

fix regex for input validation

9a6331c

kenjis requested changes Aug 14, 2022

View reviewed changes

fix: regex for input validation

255f784

- combine two validation to one while loop

kenjis reviewed Aug 18, 2022

View reviewed changes

system/CLI/CLI.php Outdated Show resolved Hide resolved

kenjis reviewed Aug 18, 2022

View reviewed changes

user_guide_src/source/cli/cli_library/023.php Show resolved Hide resolved

Apply suggestions from code review

6f88d82

Co-authored-by: kenjis <[email protected]>

kenjis approved these changes Aug 18, 2022

View reviewed changes

kenjis merged commit 116f463 into codeigniter4:4.3 Aug 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new method `promptByMultipleKeys()` in CLI class #6302

Add new method `promptByMultipleKeys()` in CLI class #6302

rivalarya commented Jul 27, 2022 •

edited by kenjis

Loading

paulbalandan left a comment

paulbalandan Jul 29, 2022

paulbalandan Jul 29, 2022

paulbalandan Jul 29, 2022

paulbalandan Jul 29, 2022

kenjis Jul 29, 2022

paulbalandan Jul 29, 2022

kenjis Aug 3, 2022

paulbalandan Aug 3, 2022

kenjis commented Aug 5, 2022

rivalarya commented Aug 5, 2022

kenjis commented Aug 10, 2022

kenjis commented Aug 10, 2022

kenjis left a comment

rivalarya commented Aug 10, 2022

kenjis commented Aug 10, 2022

rivalarya commented Aug 10, 2022

kenjis commented Aug 10, 2022 •

edited

Loading

kenjis commented Aug 14, 2022 •

edited

Loading

kenjis left a comment

kenjis commented Aug 18, 2022 •

edited

Loading

kenjis commented Aug 18, 2022

rivalarya commented Aug 18, 2022

kenjis left a comment

Add new method promptByMultipleKeys() in CLI class #6302

Add new method promptByMultipleKeys() in CLI class #6302

Conversation

rivalarya commented Jul 27, 2022 • edited by kenjis Loading

paulbalandan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented Aug 5, 2022

rivalarya commented Aug 5, 2022

kenjis commented Aug 10, 2022

kenjis commented Aug 10, 2022

kenjis left a comment

Choose a reason for hiding this comment

rivalarya commented Aug 10, 2022

kenjis commented Aug 10, 2022

rivalarya commented Aug 10, 2022

kenjis commented Aug 10, 2022 • edited Loading

kenjis commented Aug 14, 2022 • edited Loading

kenjis left a comment

Choose a reason for hiding this comment

kenjis commented Aug 18, 2022 • edited Loading

kenjis commented Aug 18, 2022

rivalarya commented Aug 18, 2022

kenjis left a comment

Choose a reason for hiding this comment

Add new method `promptByMultipleKeys()` in CLI class #6302

Add new method `promptByMultipleKeys()` in CLI class #6302

rivalarya commented Jul 27, 2022 •

edited by kenjis

Loading

kenjis commented Aug 10, 2022 •

edited

Loading

kenjis commented Aug 14, 2022 •

edited

Loading

kenjis commented Aug 18, 2022 •

edited

Loading