click-contrib / click_params

Bunch of click parameters to use
Apache License 2.0
31 stars 5 forks source link

Add ignore empty to choice list #16

Open saroad2 opened 2 years ago

saroad2 commented 2 years ago

Description

Added the ignore_empty parameter to ChoiceParamListType

codecov-commenter commented 2 years ago

Codecov Report

Merging #16 (90de4df) into master (85d2bfb) will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #16   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         336    336           
=====================================
  Hits          336    336           
Impacted Files Coverage Δ
click_params/miscellaneous.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 85d2bfb...90de4df. Read the comment docs.

saroad2 commented 2 years ago

@lewoudar , It would be nice if after you review and approve this you will deploy a new version of click_params. It will reduce few lines of code in my repos 😃

lewoudar commented 2 years ago

@saroad2 to be honest, when I first saw your PR introducing ChoiceParamListType I first asked myself why he didn't add ignore_empty and after a few minutes, I said "aww it is clever!" because the goal of choices is to choose values in the given list. So I don't see a use case to accept an empty list for a ChoiceParamListType. I think it is going against the nature of click.Choice. If no choice is possible then the option / argument using it should be optional.

saroad2 commented 2 years ago

Actually, the reason I didn't add ignore_empty to ChoiceListParamType is that the two PRs were written simultaneously 😂 . Only after both where merged I figured I forgot to add that option to ChoiceListParamType.

The absence of ignore_empty is actullay a bug as one can see in the following example:

choices = click.prompt(
    "Enter your choices",
    type=click_params.ChoiceListParamType(["a", "b", "c"]),
    default="",
)

If you press only Enter (meaning default will be chosen) a BadParameter error will be raised. This was the initial reason I added the ignore_empty option.

I think that this PR should be merged. sometimes our users want to choose a list from a subset of options, but an empty list is valid. The only constraint is that once the list has a value, it must be from a closed set of options.

I my own use case one can mark a "command" with a subset of "contexts", where "contexts" are a closed set of possible values. If no contexts are chosen, the "command" is set without any "contexts", and it's a valid option.

lewoudar commented 2 years ago

Hi @saroad2 , sorry for the very late answer, I was busy with other projects and forgot looking again for this, but it is not really an excuse.

The absence of ignore_empty is actullay a bug as one can see in the following example:

choices = click.prompt(
    "Enter your choices",
    type=click_params.ChoiceListParamType(["a", "b", "c"]),
    default="",
)

I just test it and I don't see a bug, you will just get stuck in the prompt. Again, since click.Choice does not accept your use case, I don't think I will add this feature in click_params, I'm sorry.

BTW, testing the following code, I notice that the choices don't appear neither in the documentation, nor in the prompt, any idea why?

import click
import click_params

@click.command()
@click.option('-f', '--fruit', type=click_params.ChoiceListParamType(['apple', 'pineapple']), prompt=True)
def cli(fruit):
    click.echo(fruit)
    fruit = click.prompt('another fruit choice', type=click_params.ChoiceListParamType(['strawberry', 'water melon']))
    click.echo(fruit)

if __name__ == '__main__':
    cli()