Skip to content
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

Feature: CLI: add promptByKey method #3912

Merged

Conversation

element-code
Copy link
Contributor

@element-code element-code commented Nov 19, 2020

Allows the developer to prompt for a key-based selection of a value.
This requires to have #3911 merged to complete the PHPStan Task.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@element-code
Copy link
Contributor Author

This needs a user guide update, feel free to commit

@lonnieezell
Copy link
Member

This seems to be very close in functionality to the existing CLI::prompt() as it already allows you to choose from a list. I don't see it as being significantly different from existing functionality, so closing. Feel free to comment if I'm wrong, though.

@lonnieezell lonnieezell closed this Dec 8, 2020
@element-code
Copy link
Contributor Author

The difference to prompt is that prompt allows label or value-based selection while promtSelect allows key-based selection.
E.g. when providing options to select one option by an easy-to-type key and not by its full description.

@element-code
Copy link
Contributor Author

@lonnieezell can I get your opinion on this with the updated explanation?

@paulbalandan
Copy link
Member

The difference to prompt is that prompt allows label or value-based selection while promtSelect allows key-based selection.
E.g. when providing options to select one option by an easy-to-type key and not by its full description.

I don't quite understand what you mean here. Can you give a visual example to this?

@element-code
Copy link
Contributor Author

Basically promptSelect is like prompt but key-based.

promptSelect allows to provide the user easy-to-type keys whilst provinding a seperate easy-to-understand description of the option.
Also this allows to select a programmatically easy value for a translated description.

$selectedKey = CLI::promptSelect('Select a type`, [
  'foo' => 'Description of foo',
  'bar' => 'Description of bar',
  'foobar' => 'Description of foobar',
]);
// $selectedKey: 'bar'

which results in the following output:

Select a Type:
  foo        Description of foo
  bar        Description of bar
  foobar     Description of foobar

selection: bar

When trying the same with the prompt you have to make your option easy-to-type and easy-to-understand which contradicts itself in the most cases..
This works really well for simple selections like apples or bananas but not for complex selections where the user has to understand the option verry well.

$selectedKey = CLI::promptSelect('Select a type`, [
  'foo' => 'Description of foo',
  'bar' => 'Description of bar',
  'foobar' => 'Description of foobar',
]);
// $selectedKey: 'Description of bar'

which results in the following output (not really but for the sake of explaing the difference):

Select a Type:
  Description of foo
  Description of bar
  Description of foobar

selection: Description of bar

@paulbalandan
Copy link
Member

More or less I am getting the concept you want to achieve, but I want to confirm if this is different from what I am accomplishing via prompt.

In my library's updater, I am using prompts to select something but with a description for each:

Found 3 files to consolidate.
[p] Proceed.
[l] List all files to consolidate.
[c] List created files only (0).
[m] List modified files only (3).
[d] List deleted files only (0).
[a] Abort.

What shall I do? [p, l, c, m, d, a]:

@element-code
Copy link
Contributor Author

element-code commented Jan 29, 2021

Yeah that is the same Idea, but promptSelect has the following features which make it way easier to use:

  1. One simple array (key-value) based instead of duplicate code per line
  2. Auto-Padding for all keys
  3. Auto Wrapping for all descriptions in case they don't fit in one line

But going your way instead of mine but wrapped in a function seems like an option for me.

  1. Not Duplicating prompt. Instead, write the options before using prompt
  2. Use prompt afterwards on only the keys

If you think that's the better solution, I can do that!


Easy use case: Select a user permission level. You got an array of all levels & their translated descriptions and just want to select one.

$user->setPermissionLevel( CLI::promptSelect($levels) );

This would be way more complicated not using promptSelect.

@paulbalandan
Copy link
Member

Ok. But how do you make the selection? Via typing the keys or via the up/down arrow keys. IMO, if it is named promptSelect then I would be more or less expecting a CLI equivalent of an HTML dropdown list. If you will be just typing it too then I think thats why Lonnie thinks this is functionally the same with prompt.

@element-code
Copy link
Contributor Author

It is (right now) based on typing (like your own "implementation" of it).
The name originated from the HTML dropdown, the main feature is the key-based usage instead of the promts value-based usage.

@paulbalandan
Copy link
Member

paulbalandan commented Jan 29, 2021

After pondering on this for a while, I've noticed that the code is structurally the same with prompt (but with some tweaks, of course) which is why Lonnie thought of this as redundant. To eliminate code dupe, I suggest using those tweaks into the prompt code. You'll be changing the $options param. The flow is like this.

  1. First check if $options is an array and not empty.
  2. Check if this is a list (i.e., keys are all numeric) or key-based (don't know the proper term :D)
  3. If a list, continue the current prompt logic.
  4. If the key-based type, we do extraction.
    a. Extract the keys as the options for prompt.
    b. Extract the values as the description.
  5. Then display them all.

We can consider this then as an upgrade to prompt.

@element-code
Copy link
Contributor Author

element-code commented Feb 1, 2021

Seems fine for me except for one thing:
This way won't allow selecting from a numbered array e.g. when using ID's as keys (which is a common usage of this feature I guess e.g. selecting a domain from all registered domains and so on).
Introducing a new parameter to force the key-based selections seems a little overkill for me?

@paulbalandan
Copy link
Member

Hmm. I guess you're right. It seems a new method is the right way to go. Actually I want this feature in so that I can reduce some lines in my library. However, in order to promote DRY, we must ensure that the common code here with prompt is just reused. If you can split out the reusable code into a private static function that will be used by both prompt and promptSelect I think that would be great.

@element-code
Copy link
Contributor Author

I'll do that!
Please consider reopening this PR and put it in draft mode :)

@paulbalandan paulbalandan reopened this Feb 1, 2021
@element-code
Copy link
Contributor Author

element-code commented Feb 1, 2021

Works now & is DRY conform.
We could discuss about braces [foo] around the options and/or colors.

CLI::promptSelect('Select one option:', ['foo', 'bar', 'foobar']);

//Select one option:
//  0  foo
//  1  bar
//  2  foobar
//
//[0, 1, 2]: 

// -------------------------------------------------

CLI::promptSelect('Select one option:', [
  'foo' => 'Description of foo',
  'bar' => 'Description of bar',
  'foobar' => 'Description of foobar'
]);

//Select one option:
//  foo     Description of foo
//  bar     Description of bar
//  foobar  Description of foobar
//
//[foo, bar, foobar]: 

@element-code element-code marked this pull request as ready for review February 1, 2021 16:52
@element-code
Copy link
Contributor Author

I do write the user guide update when the code-basis is accepted 😀

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

For the colors, I prefer green text for the options enclosed in square brackets.

Can you make the first param of promptSelect accepts a string|array? I am thinking that in addition to your proposed setup of the question on top of the choices then the selection list at the bottom, I would like to have some "preface" to the choices, then the question will be aligned similar to the original prompt.

For example:

CLI::promptSelect(['These are your choices', 'Which would you like?'], [
    'apple' => 'The red apple',
    'orange' => 'The plump orange',
   'banana' => 'The ripe banana'
], null);

// These are your choices
//    [apple]     The red apple
//    [orange]  The plump orange
//    [banana]  The ripe banana
//
// Which would you like? ['apple', 'orange', 'banana']

So basically what would happen is two options. If you passed a string text, this would be passed as the question itself printed above the choices and a PHP_EOL is on the bottom with the selection. If I pass in an array, the first value is treated as the preface and the second value is treated as the question.

system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Outdated Show resolved Hide resolved
@paulbalandan
Copy link
Member

Please rebase again. I've merged your other PR which caused conflict with this.

@paulbalandan paulbalandan added the needs rework Changes requested by reviewer that are still pending label Feb 2, 2021
@element-code
Copy link
Contributor Author

The idea with the double texts is great, thought about it multiple times how to implement a second text for the prompt.
The only pitfall for dev's I see with this implementation is, thinking you could omit the second argument when using no texts e.g. when manually putting a "label" above -- implemented it anyways.

@paulbalandan
Copy link
Member

The idea with the double texts is great, thought about it multiple times how to implement a second text for the prompt.
The only pitfall for dev's I see with this implementation is, thinking you could omit the second argument when using no texts e.g. when manually putting a "label" above -- implemented it anyways.

If using no second text, he should only supply a string text. And it will give that text to the preface and the question will still be using PHP_EOL, just like what you did.

@element-code
Copy link
Contributor Author

CLI::promptSelect(['Found 3 files to consolidate.', 'What shall I do?'], [
  'foo' => 'Climb leg rub face on everything give attitude nap all day for under the bed. Chase mice attack feet but rub face on everything hopped up on goofballs. ',
  'bar' => 'European minnow priapumfish mosshead warbonnet shrimpfish bigscale. Cutlassfish porbeagle shark ricefish walking catfish glassfish Black swallower. ',
  'foobar' => 'Efficiently unleash cross-media information without cross-media value. Quickly maximize timely deliverables for real-time schemas. Dramatically maintain clicks-and-mortar. '
]);
light dark
grafik grafik

I'd propose making the default option in prompt green to add better contrast. But I'm fine if we leave it as is.

@paulbalandan
Copy link
Member

Changing default to green is not breaking, so I'm fine with the change.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Just some minor CS fixes. Then lastly, the user guide addition.

I'd like to ask why change the padding to +6 from +2?

system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Outdated Show resolved Hide resolved
system/CLI/CLI.php Outdated Show resolved Hide resolved
@paulbalandan paulbalandan added the docs needed Pull requests needing documentation write-ups and/or revisions. label Feb 12, 2021
@element-code
Copy link
Contributor Author

element-code commented Feb 17, 2021

I'd like to ask why change the padding to +6 from +2?

+2 for the added square brackets
+2 moved the two leading spaces inside the str_pad call 41e749b#diff-ab1992201af81c818cedbe73d5b42d55fd1c3ac1fe1239b6dfc3018b419d5446L300

Working on the cs fixes & user guide in 2 weeks

@paulbalandan
Copy link
Member

I'd like to ask why change the padding to +6 from +2?

+2 for the added square brackets
+2 moved the two leading spaces inside the str_pad call 41e749b#diff-ab1992201af81c818cedbe73d5b42d55fd1c3ac1fe1239b6dfc3018b419d5446L300

Working on the cs fixes & user guide in 2 weeks

@element-code any update on this one? We just need a little more push to here before it's done.

@element-code
Copy link
Contributor Author

Timings change on the fly.. it's on my list; eta: after the next two weaks :(

@element-code element-code force-pushed the feature-cli-added-prompt-select branch from b759abb to 8937977 Compare August 2, 2021 12:20
@element-code
Copy link
Contributor Author

Finally, found time for this

  • CS fixed
  • rebased
  • squashed commits
  • docs added

When committing and running git hooks it runs on the full code.
CS Fixer reports errors in files which aren't changed and are obviously out of topic, which prevents the commit from happen.
This results for me in unchecking the "run git hooks" checkbox, bc I can't commit without doing that.

Files in app, admin, or public are not following the coding standards. Please fix them before commit.

How is this supposed to be and how to work with that in the future?

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Looking good already. Thanks for taking some time to finish this. However, develop is currently locked for styling and CS changes only. I'm afraid you need to rebase your branch against 4.2

For your concern on CS running on whole codebase is intended as after develop gets a release, all code should now be compliant. You can instead run it manually:

vendor/bin/php-cs-fixer fix -v file1 file2 file 3

Then, when committing just add the --no-verify flag.

user_guide_src/source/cli/cli_library.rst Outdated Show resolved Hide resolved
user_guide_src/source/cli/cli_library.rst Outdated Show resolved Hide resolved
@element-code element-code force-pushed the feature-cli-added-prompt-select branch 2 times, most recently from 434fd07 to a8a13b1 Compare August 2, 2021 14:32
@element-code element-code changed the base branch from develop to 4.2 August 2, 2021 14:33
@element-code element-code force-pushed the feature-cli-added-prompt-select branch from a8a13b1 to 11e4772 Compare August 2, 2021 14:41
@paulbalandan paulbalandan removed docs needed Pull requests needing documentation write-ups and/or revisions. needs rework Changes requested by reviewer that are still pending labels Aug 2, 2021
@paulbalandan
Copy link
Member

Thank you, @element-code

@paulbalandan paulbalandan merged commit 78a0d25 into codeigniter4:4.2 Aug 2, 2021
@element-code element-code changed the title Feature: CLI: Select: added select prompt Feature: CLI: add promptByKey method Aug 2, 2021
@element-code element-code deleted the feature-cli-added-prompt-select branch August 2, 2021 20:33
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.

3 participants