Support configuring submit action key in select #163

Geekfish · 2021-05-14T11:36:10Z

Describe the change

Attempting to address the issue described here, if merged this MR will:

Change #select and #multi_select to accept a :confirm_keys option
Change #multi_select to also accept a :select_keys option
Maintain the default behaviour submit keys of:
- :space and :return for #select
- :return for #multi_select
Fix the #select and #multi_select hints to print out the correct keys (previously only printed "Enter").
Fix and update the README.md hint examples for #select and #multi_select

Why are we doing this?

This is particularly useful for users of the filter option, as the current behaviour doesn't allow the user to use spaces in their input.

Benefits

Makes the selection filtering behaviour more flexible.

Drawbacks

One more configuration option to maintain?
~~Since this relies on the key name rather than the value, specific alphanumeric values are not supported.~~ This has been addressed now, alphanumerics are also supported.

Requirements

Geekfish · 2021-05-14T11:38:59Z

Looks like this is breaking multi_select tests now, I'll fix and update.

Geekfish · 2021-05-14T16:38:58Z

I think it's ready now.

All feedback on this would be super-appreciated (especially since I'm not very well-versed in Ruby)!

It should be easier to review this commit-by-commit, but they can probably all be merged in the end.

piotrmurach

Hi Eleni 👋

Thank you for tackling this issue! Apart from small comments, overall, the implementation looks great. It was a pleasure to review.

My main points are:

to consider another alternative key in place of :tab for all the examples and tests,
add :enter to the list of recognised default keys
see if select_key can accept more than one key

It's up to you really if you wish to split this commit up into a smaller one that deals only with the readme fixes and another that handles the keys specification or you prefer to keep it all together.

README.md

lib/tty/prompt/list.rb

lib/tty/prompt/multi_list.rb

examples/select_filtered_with_spaces.rb

Geekfish · 2021-05-15T12:05:42Z

Thank you for the feedback @piotrmurach I'll look into making the suggested updates!

lib/tty/prompt/list.rb

Geekfish · 2021-05-16T16:47:47Z

Hey @piotrmurach ! I have tried to address all the points above, again it may be easier to review per-commit 🙂

Geekfish · 2021-05-16T16:49:47Z

lib/tty/prompt/list.rb

+        if %i[return enter].include?(key_name)
+          "Enter"
+        else
+          key_name.to_s.split("_").map(&:capitalize).join("+")


I thought this was a sensible default, as it covers nicely printing most ctrl/shift+ combination keys. The custom labels should allow the user to deal with more thorny cases.

Like it. This works very well for the majority of the cases.

lib/tty/prompt/list.rb

piotrmurach

This is looking good. Functionality-wise we're there. I reviewed everything and found few 'nitpicky' things. Let me know what you think and thank you for working on this feature!

README.md

piotrmurach · 2021-05-17T09:51:38Z

lib/tty/prompt/list.rb

+        if %i[return enter].include?(key_name)
+          "Enter"
+        else
+          key_name.to_s.split("_").map(&:capitalize).join("+")


Like it. This works very well for the majority of the cases.

lib/tty/prompt/list.rb

lib/tty/prompt/multi_list.rb

lib/tty/prompt/list.rb

Geekfish · 2021-08-20T20:39:53Z

Hey @piotrmurach, I have addressed the last round of feedback. Let me know what you think!

piotrmurach · 2021-08-21T11:42:27Z

Hi @Geekfish, nice one! Nearly there. There are still few issues left to do with the yardoc. I think it's best if you run the yard tool and see the generated docs yourself. Please run yard doc lib/tty/prompt/list.rb to generate doc/index.html. It'd be good to get this produce no errors when compiling the docs for the list.rb page and readme. Later on, when the gem is released these docs will end up on rubydoc tty-prompt page.

Geekfish · 2021-08-21T13:56:40Z

Hey @piotrmurach ! Thanks for the instructions above, I'd never used yard before, I'll fix the warnings.

How do you feel about adding these to the gemspec?

spec.add_development_dependency "yard"
spec.add_development_dependency "rubocop"

+ some extra instructions for contributing?

Would it also make sense to have something like pronto-rubocop to check for rubocop issues only on the diffs?

Maybe we could do something in a separate PR, I would be happy to give a hand to make contributing easier and require a bit less work from you having to comb through everything :)

Geekfish · 2021-08-21T14:27:34Z

@piotrmurach I believe I have fixed all the list/multi_list warnings now.

piotrmurach · 2021-08-21T21:22:40Z

@Geekfish I can confirm all the warnings for list/multi_list are gone. The yard tool gives few warnings for the README.md. It seems that it struggles to parse text immediately followed by code fences. When you run yard doc README.md you will see syntax errors and can view the generated doc and the :confirm_keys sections to see the problem.

As for adding yard and rubocop to gem dependencies, I'm for keeping them out of the gemspec. I'd prefer to use GitHub Actions to automate the quality checks, ensure the changelog is updated etc. I think the PR checks can be largely automated this way. Currently, I use HoundCI to help with rubocop checks but it can become quite noisy when reviewing. Whatever the approach I'd like to copy it to all my tty open-source projects.

I'm in favour of adding, for example, CONTRIBUTING.md that then could form the basis for all the tty projects. Any help here would be great.

Geekfish · 2021-08-22T10:25:26Z

Hey @piotrmurach,

I'm not sure if we're running different versions of yard or something, but I'm not getting any of the README.md errors you are mentioning in the output, even when I run it as suggested. I'm only getting:

[warn]: Syntax error in `readme.md`:(1,0): syntax error, unexpected '<'

I manually inspected the README and fixed two types of issues I identified with extra backticks and missing newlines (not only in new docs).

Am I missing something? Is there a flag / extra gem that I'm missing, so I can get those warnings?
I'm running yard 0.9.26.

When I run yard for the entire doc set, I only am getting a few other warnings for prompt.rb and a handful of other places but still no warnings on README. However it does render the README as expected...

Geekfish · 2021-08-22T10:27:59Z

For clarity, this is my output:

$ bundler exec yard doc
[warn]: @param tag has unknown parameter name: :input
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :output
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :env
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :symbols
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: options
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :prefix
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :interrupt
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :track_history
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :enable_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :active_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :help_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :error_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :string
    in file `lib/tty/prompt.rb' near line 156
[warn]: @param tag has unknown parameter name: :colors
    in file `lib/tty/prompt.rb' near line 156
[warn]: @param tag has unknown parameter name: choices
    in file `lib/tty/prompt.rb' near line 285
[warn]: @param tag has unknown parameter name: choices
    in file `lib/tty/prompt.rb' near line 305
[warn]: @param tag has unknown parameter name: choices
    in file `lib/tty/prompt.rb' near line 325
[warn]: @param tag has unknown parameter name: messages
    in file `lib/tty/prompt.rb' near line 437
[warn]: @param tag has unknown parameter name: messages
    in file `lib/tty/prompt.rb' near line 453
[warn]: @param tag has unknown parameter name: messages
    in file `lib/tty/prompt.rb' near line 469
[warn]: Unknown tag @retrun in file `lib/tty/prompt.rb` near line 484
[warn]: Unknown tag @examples in file `lib/tty/prompt/choice.rb` near line 29
[warn]: @param tag has unknown parameter name:
    in file `lib/tty/prompt/choice.rb' near line 49
[warn]: @param tag has unknown parameter name:
    in file `lib/tty/prompt/choice.rb' near line 67
[warn]: Unknown tag @apu in file `lib/tty/prompt/slider.rb` near line 166
[warn]: @param tag has unknown parameter name: rule
    in file `lib/tty/prompt/question.rb' near line 323
[warn]: @param tag has unknown parameter name:
    in file `lib/tty/prompt/enum_list.rb' near line 147
[warn]: @param tag has unknown parameter name: options
    in file `lib/tty/prompt/statement.rb' near line 31
[warn]: @param tag has unknown parameter name: suggestions
    in file `lib/tty/prompt/suggestion.rb' near line 89
[warn]: @param tag has unknown parameter name: name
    in file `lib/tty/prompt/converter_registry.rb' near line 42
Files:          35
Modules:         8 (    7 undocumented)
Classes:        35 (   12 undocumented)
Constants:      39 (   21 undocumented)
Attributes:     37 (    8 undocumented)
Methods:       266 (   68 undocumented)
 69.87% documented

and README only:

$ bundler exec yard doc README.md
[warn]: Syntax error in `readme.md`:(1,0): syntax error, unexpected '<'
Files:           0
Modules:         0 (    0 undocumented)
Classes:         0 (    0 undocumented)
Constants:       0 (    0 undocumented)
Attributes:      0 (    0 undocumented)
Methods:         0 (    0 undocumented)
100.00% documented

piotrmurach · 2021-08-22T10:59:11Z

@Geekfish The README.md seems to be fixed now and looks ok. Thank you! We're both running the same version of yard. I can currently see only one syntax error similar to yourself which we can safely ignore. As far as I can tell your last commit has fixed all the issues in the readme. If you temporarily undo your last commit and run yard doc README.md you will see few warnings like:

[warn]: In file `README.md':961: Cannot resolve link to ctrl_s: from text:
	...{ctrl_s: "Ctrl-S"}...
[warn]: In file `README.md':961: Cannot resolve link to "," from text:
	...{"," => "Comma (,)"}...
[warn]: In file `README.md':961: Cannot resolve link to ctrl_s: from text:
	...{ctrl_s: "Ctrl-S"}...
[warn]: In file `README.md':961: Cannot resolve link to "," from text:
	...{"," => "Comma (,)"}...
etc...

I can also confirm that I'm getting exactly the same output from the yard for the entire project matching yours. So all is good.

Geekfish · 2021-08-22T19:18:18Z

Hey @piotrmurach!
It's still a bit weird, I never got the warnings above. Even when I check out pre-fix commits, for example commit 2334b89d5fa33b130d8d42d0ddcb8cac255af8e3:

21:12:42 ~/workspace/tty-prompt > 2334b89 ✘ ✹ ✭ 
$ bundler exec yard doc
[warn]: @param tag has unknown parameter name: :input
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :output
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :env
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :symbols
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: options
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :prefix
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :interrupt
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :track_history
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :enable_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :active_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :help_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :error_color
    in file `lib/tty/prompt.rb' near line 120
[warn]: @param tag has unknown parameter name: :string
    in file `lib/tty/prompt.rb' near line 156
[warn]: @param tag has unknown parameter name: :colors
    in file `lib/tty/prompt.rb' near line 156
[warn]: @param tag has unknown parameter name: choices
    in file `lib/tty/prompt.rb' near line 285
[warn]: @param tag has unknown parameter name: choices
    in file `lib/tty/prompt.rb' near line 305
[warn]: @param tag has unknown parameter name: choices
    in file `lib/tty/prompt.rb' near line 325
[warn]: @param tag has unknown parameter name: messages
    in file `lib/tty/prompt.rb' near line 437
[warn]: @param tag has unknown parameter name: messages
    in file `lib/tty/prompt.rb' near line 453
[warn]: @param tag has unknown parameter name: messages
    in file `lib/tty/prompt.rb' near line 469
[warn]: Unknown tag @retrun in file `lib/tty/prompt.rb` near line 484
[warn]: Unknown tag @examples in file `lib/tty/prompt/choice.rb` near line 29
[warn]: @param tag has unknown parameter name:
    in file `lib/tty/prompt/choice.rb' near line 49
[warn]: @param tag has unknown parameter name:
    in file `lib/tty/prompt/choice.rb' near line 67
[warn]: Unknown tag @apu in file `lib/tty/prompt/slider.rb` near line 166
[warn]: @param tag has unknown parameter name: rule
    in file `lib/tty/prompt/question.rb' near line 323
[warn]: @param tag has unknown parameter name:
    in file `lib/tty/prompt/enum_list.rb' near line 147
[warn]: @param tag has unknown parameter name: options
    in file `lib/tty/prompt/statement.rb' near line 31
[warn]: @param tag has unknown parameter name: suggestions
    in file `lib/tty/prompt/suggestion.rb' near line 89
[warn]: @param tag has unknown parameter name: name
    in file `lib/tty/prompt/converter_registry.rb' near line 42
Files:          35
Modules:         8 (    7 undocumented)
Classes:        35 (   12 undocumented)
Constants:      39 (   21 undocumented)
Attributes:     37 (    8 undocumented)
Methods:       266 (   68 undocumented)
 69.87% documented

piotrmurach · 2021-08-22T21:59:51Z

@Geekfish It seems that running the yard executable directly without bundler produces the readme errors. However, when I run it via bundler I don't get to see the readme errors and also it takes much longer to process documentation. I don't have a good answer why the difference in behaviour.

Geekfish · 2021-08-23T07:18:45Z

Interesting, via bundler is definitely a lot slower, as you said, but the output is the same for me, with or without bundler. Strange!

piotrmurach

Noticed a couple of small readme issues. I'd also like to add keys yard parameter descriptions for consistency with other methods. I have added them in the comments. This is the end of the amending.

lib/tty/prompt/list.rb

lib/tty/prompt/multi_list.rb

lib/tty/prompt/list.rb

README.md

piotrmurach · 2021-08-24T20:47:46Z

@Geekfish I dug a little into the yard source and couldn't identify any flags that would cause different behaviour. However, I think the reason may be that we're using different Ruby versions. When I run the yard command with Ruby 2.7.1 I could see all the warnings displayed for the readme. However, when I switched over to Ruby 3.0.0 all the warnings disappeared. This potentially implies a bug in yard tool itself. Hopefully, the Ruby version difference solves this mystery.

Geekfish · 2021-08-25T17:47:13Z

Hey @piotrmurach ! This yard thing is super weird, I did try with ruby 2.6.5 yesterday, still no errors!
That aside, I have fixed the latest batch of issues 🙂

piotrmurach · 2021-08-25T18:29:10Z

@Geekfish Strangely, these errors are not present on Ruby 2.6.5. Only 2.7.x triggers them for me. Thank you for the changes!

fdiaz · 2021-09-06T12:19:57Z

@piotrmurach just a friendly ping to know when this is planned to be merged to update.

dishcandanty · 2023-07-02T20:09:25Z

Would be really nice to have this feature :)

rafael-assis · 2023-12-05T20:21:52Z

Hi @piotrmurach.

I work at Airbnb and we use tty-prompt in our ruby tooling here.

I came across the same issue today and found that the problem is already solved in this PR.
Is there anything missing for this PR to be merged and the feature released?

I'd love to help if there's anything I can do for it.

Thanks in advance!

Add test that demonstrates default list select behaviour

8ac4e83

Geekfish changed the title ~~Add test that demonstrates default list select behaviour~~ Support configuring submit action key in select May 14, 2021

Geekfish force-pushed the feature/config-selection branch 3 times, most recently from d9cb837 to d0bfe5a Compare May 14, 2021 16:13

Geekfish added 3 commits May 14, 2021 18:25

Fix typo in slider and list show_help

993ced2

Support submit_keys and select_key in (multi_)select

44a179f

Fix hints for #select and #multi_select in README

dce79bf

Geekfish force-pushed the feature/config-selection branch from d0bfe5a to dce79bf Compare May 14, 2021 16:32

piotrmurach reviewed May 15, 2021

View reviewed changes

Geekfish added 2 commits May 16, 2021 15:16

Rename select to select_choice

0a99a13

Add enter to default submit keys

9cb0352

Geekfish commented May 16, 2021

View reviewed changes

lib/tty/prompt/list.rb Outdated Show resolved Hide resolved

Geekfish force-pushed the feature/config-selection branch 2 times, most recently from dfc515c to 636da64 Compare May 16, 2021 16:45

Geekfish commented May 16, 2021

View reviewed changes

Geekfish added 4 commits May 16, 2021 19:00

Replace :tab with :ctrl_s as submit key in examples, docs and tests

5327ce9

Allow defining the label of submit keys

b24819d

Add author handle in CHANGELOG

135e62a

Make multi-select select keys an array and allow custom labels

395e6b1

Geekfish force-pushed the feature/config-selection branch 2 times, most recently from 86dee52 to ecd8bbb Compare May 16, 2021 17:19

Ensure cross-system compat for EOL chars in custom action keys

3fa9f2c

Geekfish force-pushed the feature/config-selection branch from ecd8bbb to 3fa9f2c Compare May 16, 2021 17:48

Geekfish commented May 16, 2021

View reviewed changes

lib/tty/prompt/list.rb Outdated Show resolved Hide resolved

piotrmurach reviewed May 17, 2021

View reviewed changes

Geekfish force-pushed the feature/config-selection branch from fad599f to 39ed0d1 Compare August 20, 2021 20:38

Geekfish added 3 commits August 20, 2021 22:51

Make submit and select examples consistent, add missing yard option

b43a32a

Change conflicting keys error message

d42efff

Use helpers for confirm_keys DSL test

9dea17b

Geekfish force-pushed the feature/config-selection branch from 39ed0d1 to 9dea17b Compare August 20, 2021 20:51

Geekfish force-pushed the feature/config-selection branch from fb77935 to 73c5839 Compare August 21, 2021 14:22

Fix yard warnings in list.rb and multi_list.rb

7dc89e8

Geekfish force-pushed the feature/config-selection branch from 73c5839 to 7dc89e8 Compare August 21, 2021 14:24

Fix hint description in README

2334b89

Geekfish added 2 commits August 22, 2021 11:56

Remove superfluous backticks from README code blocks

44945c4

Add missing newlines before code blocks in README

33b4beb

piotrmurach reviewed Aug 24, 2021

View reviewed changes

Fix help text in README and add key @param descriptions

79cd017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support configuring submit action key in select #163

Support configuring submit action key in select #163

Geekfish commented May 14, 2021 •

edited

Geekfish commented May 14, 2021

Geekfish commented May 14, 2021 •

edited

piotrmurach left a comment

Geekfish commented May 15, 2021

Geekfish commented May 16, 2021

Geekfish May 16, 2021

piotrmurach May 17, 2021

piotrmurach left a comment

piotrmurach May 17, 2021

Geekfish commented Aug 20, 2021

piotrmurach commented Aug 21, 2021

Geekfish commented Aug 21, 2021

Geekfish commented Aug 21, 2021

piotrmurach commented Aug 21, 2021

Geekfish commented Aug 22, 2021 •

edited

Geekfish commented Aug 22, 2021

piotrmurach commented Aug 22, 2021

Geekfish commented Aug 22, 2021

piotrmurach commented Aug 22, 2021

Geekfish commented Aug 23, 2021 •

edited

piotrmurach left a comment

piotrmurach commented Aug 24, 2021

Geekfish commented Aug 25, 2021

piotrmurach commented Aug 25, 2021

fdiaz commented Sep 6, 2021

dishcandanty commented Jul 2, 2023

rafael-assis commented Dec 5, 2023 •

edited

Support configuring submit action key in select #163

Are you sure you want to change the base?

Support configuring submit action key in select #163

Conversation

Geekfish commented May 14, 2021 • edited

Describe the change

Why are we doing this?

Benefits

Drawbacks

Requirements

Geekfish commented May 14, 2021

Geekfish commented May 14, 2021 • edited

piotrmurach left a comment

Choose a reason for hiding this comment

Geekfish commented May 15, 2021

Geekfish commented May 16, 2021

Geekfish May 16, 2021

Choose a reason for hiding this comment

piotrmurach May 17, 2021

Choose a reason for hiding this comment

piotrmurach left a comment

Choose a reason for hiding this comment

piotrmurach May 17, 2021

Choose a reason for hiding this comment

Geekfish commented Aug 20, 2021

piotrmurach commented Aug 21, 2021

Geekfish commented Aug 21, 2021

Geekfish commented Aug 21, 2021

piotrmurach commented Aug 21, 2021

Geekfish commented Aug 22, 2021 • edited

Geekfish commented Aug 22, 2021

piotrmurach commented Aug 22, 2021

Geekfish commented Aug 22, 2021

piotrmurach commented Aug 22, 2021

Geekfish commented Aug 23, 2021 • edited

piotrmurach left a comment

Choose a reason for hiding this comment

piotrmurach commented Aug 24, 2021

Geekfish commented Aug 25, 2021

piotrmurach commented Aug 25, 2021

fdiaz commented Sep 6, 2021

dishcandanty commented Jul 2, 2023

rafael-assis commented Dec 5, 2023 • edited

Geekfish commented May 14, 2021 •

edited

Geekfish commented May 14, 2021 •

edited

Geekfish commented Aug 22, 2021 •

edited

Geekfish commented Aug 23, 2021 •

edited

rafael-assis commented Dec 5, 2023 •

edited