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

Add ignore empty to choice list #16

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

Conversation

saroad2
Copy link
Contributor

@saroad2 saroad2 commented Apr 11, 2022

Description

Added the ignore_empty parameter to ChoiceParamListType

@codecov-commenter
Copy link

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

saroad2 commented Apr 11, 2022

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

lewoudar commented Apr 13, 2022

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

saroad2 commented Apr 14, 2022

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

lewoudar commented Aug 2, 2022

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()

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

Successfully merging this pull request may close these issues.

3 participants