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

Support configuring submit action key in select #163

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

Geekfish
Copy link

@Geekfish Geekfish commented May 14, 2021

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

  • Tests written & passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated?
  • Changelog updated?

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

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

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

Geekfish commented May 14, 2021

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.

Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
examples/select_filtered_with_spaces.rb Show resolved Hide resolved
@Geekfish
Copy link
Author

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

lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
@Geekfish Geekfish force-pushed the feature/config-selection branch 2 times, most recently from dfc515c to 636da64 Compare May 16, 2021 16:45
@Geekfish
Copy link
Author

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

@Geekfish Geekfish force-pushed the feature/config-selection branch 2 times, most recently from 86dee52 to ecd8bbb Compare May 16, 2021 17:19
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
if %i[return enter].include?(key_name)
"Enter"
else
key_name.to_s.split("_").map(&:capitalize).join("+")
Copy link
Owner

Choose a reason for hiding this comment

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

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

lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/list.rb Show resolved Hide resolved
lib/tty/prompt/list.rb Show resolved Hide resolved
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/list.rb Outdated Show resolved Hide resolved
@Geekfish
Copy link
Author

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

@piotrmurach
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

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

@piotrmurach
Copy link
Owner

@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
Copy link
Author

Geekfish commented Aug 22, 2021

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
Copy link
Author

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
Copy link
Owner

@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
Copy link
Author

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
Copy link
Owner

@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
Copy link
Author

Geekfish commented Aug 23, 2021

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

Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
lib/tty/prompt/list.rb Show resolved Hide resolved
lib/tty/prompt/list.rb Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Show resolved Hide resolved
lib/tty/prompt/list.rb Show resolved Hide resolved
lib/tty/prompt/list.rb Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@piotrmurach
Copy link
Owner

@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
Copy link
Author

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
Copy link
Owner

@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
Copy link

fdiaz commented Sep 6, 2021

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

@dishcandanty
Copy link

Would be really nice to have this feature :)

@rafael-assis
Copy link

rafael-assis commented Dec 5, 2023

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!

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

Successfully merging this pull request may close these issues.

5 participants