piotrmurach / tty-prompt

A beautiful and powerful interactive command line prompt
https://ttytoolkit.org
MIT License
1.47k stars 136 forks source link

Adds a 'none of the above' option to the MultiSelect prompt #174

Closed etozzato closed 3 years ago

etozzato commented 3 years ago

Describe the change

When initializing a multi_select prompt, accepts a :reset_choice parameter. The reset_choice will modify the behaviour of the prompt: when selected, all other selections will be deselected.

drinks = %w[vodka beer wine whisky bourbon none]
prompt.multi_select("Choose your favourite drink?", drinks, reset_choice: "none")

clear_all

Why are we doing this?

It seems to be a common practice in research to provide a 'none of the above' option rather than submitting an empty multiple-choice form.

Benefits

This will make the multi_select prompt move flexible.

Drawbacks

None, this is an optional parameter.

Requirements

etozzato commented 3 years ago

Perhaps the lookup and validation logic should be the same as the default parameter

piotrmurach commented 3 years ago

Hi Emanuele 👋

Thank you for using tty-prompt and contributing!

I evaluated your PR carefully and the impact and desirability of the feature and here are my thoughts.

It seems to be a common practice in research to provide a 'none of the above' option rather than submitting an empty multiple-choice form.

In the research world, this may be a common practice, but the question is whether it makes sense when building software and in particular command-line applications? Is having an explicit 'none' option in the context of multi_select common requirement? And even if it is, I wonder if this is a good user experience that other choices are 'wiped out' when 'none' is selected. This is not obvious. Why would you select few choices after reading the question to then wish to press 'none' and for the remaining choices to be unchecked? I'd think you would read choices first and then select appropriately. What if you selected 'none' by mistake and many choices are cleared now? This seems annoying at best.

None, this is an optional parameter.

There are always drawbacks to any feature. The obvious point is that there will be another option that increases the cost of maintenance and brings potential corner cases that are not considered yet. The more important question is whether the good points outweigh the potential drawbacks here. I haven't yet had anyone express the need for such a feature which indicates that it is not that common of a problem. Now, if I were to accept this code then everyone using the prompt whether they opted in or out for the feature, would pay the performance penalty when selecting choices. There are devs that use this with potentially hundreds of choices. So there is that.

I hope you see that I'm guided by the interest of the project and all users currently using it. Therefore, I need to strike a balance between accepting feature requests that work for many people vs working in some cases. This one seems to fall into the latter category. The way it is currently, you can still have the 'none' option albeit without 'automatic' clearing.

etozzato commented 3 years ago

Thank you @piotrmurach, I appreciate the exhaustive feedback on the proposed changes.

feature requests that work for many people vs working in some cases This is a valid point!

whether they opted in or out for the feature, would pay the performance penalty when selecting choices. Good observation. The code should have been at least optimized to the point of not interfering with those that did not use the new feature.

🏳️ Closing this PR