Support configuring submit action key in select

[Geekfish]

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

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

[Geekfish]

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

[Geekfish]

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.

[Geekfish]

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

[Geekfish]

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

[Geekfish]

Hey @piotrmurach, I tried to tackle the points you raised, nitpicks are also fine, we can keep things tidy. Thanks again for the feedback.

[piotrmurach]

@Geekfish Please be assured I haven't forgotten about this and it's still on my list. My attention went temporarily to releasing other tty projects. I will do a final review shortly.

[Geekfish]

No worries @piotrmurach, thanks for the update!

[Geekfish]

Hey @piotrmurach, just a quick ping on this for when you have a bit of time to have a look at the changes :)

[piotrmurach]

Hi @Geekfish, I will review shortly as I'm easing myself back into things. I was in an accident, hence my absence from open source for some time. I was meant to review on the next day after your commits but life had different plans in store for me.

[Geekfish]

@piotrmurach Woah, that sounds really rough! No time-stress about the PR, I only pinged in case it had fallen through the cracks. Even when everything is fine, it's still something you do for free on your free time. All the best for a quick recovery!

[Geekfish]

Thank you for the feedback @piotrmurach !

I have made the smaller changes.

Regarding Enum Select, I had a look at the implementation. Even though it's very similar to simple Select, it looks like they don't really share much of the implementation (it doesn't inherit from list, it kind of re-implements everything).

A big difference is that there is no displaying/configuring of help text for the keys, so using the same params as select or multi_select, as in the example

confirm_keys: [:enter, {escape: "ESC"}, {"," => "Comma (,"}]

wouldn't make sense functionally (nothing would display the labels). On the other hand, if we re-implemented a simpler version (ex. only [:enter, :escape, ","], no labels allowed), then we break consistency.

I wonder if it would make sense to leave it for now, and if there is demand for enum_select getting more features, then we could look into refactoring it a bit to share the logic and add the help text and the rest. What do you think?

[piotrmurach]

As for enum_select prompt. Agreed, not having help text and labels makes adding :confirm_keys far less useful. My thoughts were to fully allow what the :confirm_keys accepts in the other prompts, skip using the labels and only configure confirmation keys. However, at the moment this seems like a weak proposition. As you said, this option may actually never be needed by users. I will consider it a little bit more but I must admit that I'm swaying strongly in your side of the argument. Regardless, let's focus on merging what we have already in the PR to the main branch.

[Geekfish]

Hey @piotrmurach

So I had a look at this jruby-head thing. I "fixed" it, but the actual cause was really weird. I've added it as a separate commit, if you want to have a look.

For some reason, only on jruby-head, this sort of test fails:

    it "tests jruby-head" do
      var_a = []
      result = case var_a
      when []
        true
      else
        false
      end

      expect(result).to eq(true)
    end

     Failure/Error: expect(result).to eq(true)

       expected: true
            got: false

       (compared using ==)

       Diff:
       @@ -1 +1 @@
       -true
       +false

it seems to be specific to matching empty list, other types of matches succeed as expected. I could not replicate it in other ruby versions, not even on jruby-9.2.19.0.

[piotrmurach]

@Geekfish Thank you for investigating! That's a weird way for the case to be broken on jruby and well done on figuring it out. I believe we're ready to merge? Any suggestions for descriptive commit subject/body?

[Geekfish]

@piotrmurach how does this sound?

Add custom key support to select and multi_select

- Add :confirm_keys option to both `select` and `multi_select`
- Add :select_keys option to `multi_select`
- Add examples for the above
- Update documentation code examples
- Fix some minor typos

I don't know if you also normally reference the issue/PR?

[piotrmurach]

@Geekfish Thank you. I'd change the subject line to be more specific regarding keys. There is another idea to have custom keys for selecting items which I'd like to be added as well but that's another story. When it comes to the message body I try to provide more explanation for the change. How about this?

Add custom confirmation and selection key support to select and multi_select.

Because the select prompt uses space to confirm a choice and the multi_select
prompt uses space to select a choice, it is impossible to filter a list with items
containing spaces. This change allows the user to configure different keys for
selecting or confirming a choice. It will also increase flexibility when filtering
is disabled as it will let the user pick their preferred keys over the default ones.

[Geekfish]

Hey @piotrmurach, I have added the DSL support, let me know what you think.

[Geekfish]

Hey @piotrmurach! I think I applied all the suggestions above.

It's worth noting that in the case where the first parameter is a hash, the rounded brackets are still required (I guess due to slightly ambiguous syntax?), so:

menu.confirm_keys :enter, {escape: "ESC"}, {"," => "Comma (,)"}
# or, if we want a hash first:
menu.confirm_keys({escape: "ESC"}, :enter, {"," => "Comma (,)"})

[piotrmurach]

@Geekfish Ahh yes, Ruby probably 'thinks' this is a block rather than a hash argument. All changes look good, only List#confirm_keys is missing a comment.

[Geekfish]

Hi @piotrmurach, thanks for staying on top of this. I have pushed some more fixes for the things you raised.

[Geekfish]

@piotrmurach changes pushed, let's see – Nth-time lucky 😅

[piotrmurach]

@Geekfish Thanks! As they say, the nth+1 is the charm 😄

[fdiaz]

@piotrmurach hi there! I've been following this PR since I'd love to be able to use this feature. Are there any plans to release this soon? Thanks 😉

[piotrmurach]

@fdiaz My attention has been shortly diverted to releasing other projects. I appreciate your enthusiasm for this feature and be assured it will be merged soon.

@Geekfish I do apologise, I wasn't meant to 'disappear' for so long. Maintaining many projects with competing priorities is a challenge. I'll strive to find time tomorrow.

[Geekfish]

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

[piotrmurach]

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]

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]

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

[piotrmurach]

@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]

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...