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

Fix default values of multiple options with optional values #2004

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

ikapelyukhin
Copy link
Contributor

Fixes default values for multiple options with optional values. The sentinel object (click.core._flag_needs_value) is now properly replaced by the value of flag_value.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@ikapelyukhin
Copy link
Contributor Author

Things get a bit weird with ParameterSource and multiple options: you might get some of the option values from defaults or the prompt, others specified explicitly on the command line.

Also, default values populated from flag_value have ParameterSource.COMMANDLINE rather than ParameterSource.DEFAULT, which doesn't seem right.

@davidism
Copy link
Member

Things get a bit weird with ParameterSource and multiple options: you might get some of the option values from defaults or the prompt, others specified explicitly on the command line.

That needs to be addressed, the value may only come from one source.

@ikapelyukhin
Copy link
Contributor Author

ikapelyukhin commented Jul 13, 2021

Things get a bit weird with ParameterSource and multiple options: you might get some of the option values from defaults or the prompt, others specified explicitly on the command line.

That needs to be addressed, the value may only come from one source.

Prompting issue is no longer relevant as I've reverted prompting-related changes I made

@davidism OK, I'll need some more input from you then, there are two potential cases that I'm seeing:

Without a prompt

@click.option('--foo', multiple=True, is_flag=False, flag_value='default')

So if you do ./script.py --foo --foo test you'd get ('default', 'test') as a value for foo option and the source is ParameterSource.COMMANDLINE, which is accurate for both items in the list because of how flag_value source is determined:

click/src/click/core.py

Lines 2867 to 2869 in ce152bd

else:
value = self.flag_value
source = ParameterSource.COMMANDLINE

So everything seems fine in this case as is, assuming that ParameterSource.COMMANDLINE is correct and shouldn't be ParameterSource.DEFAULT instead for one of the items.

With a prompt

@click.option('--foo', multiple=True, is_flag=False, flag_value='default', prompt="Foo")`
  1. How do you even specify multiple values with a prompt? I'm getting an Error: Value must be an iterable. for whatever input I provide. Is prompting even supposed to work for options with multiple=True?

  2. If I do ./script.py --foo --foo test, am I expected to provide values via the prompt for both of them, or just the first one that's missing a value?

  3. Provided that it is possible to use prompts with multiple=True and specify just one of the values, if I do ./script.py --foo --foo test and specify one of the values after being prompted what's the ParameterSource then? One part of the value is coming from ParameterSource.PROMPT and the other one is coming from ParameterSource.COMMANDLINE.

Seeing how it doesn't seem possible to set an iterable value via the prompt, there's no problem with ParameterSource as is here either 😅

@ikapelyukhin
Copy link
Contributor Author

@davidism OK, I've reverted the changes related to prompting, as it's not really related to the issue at hand. So things should be pretty straightforward now and source is set to ParameterSource.COMMANDLINE whenever option's default value is being used.

@davidism davidism modified the milestones: 8.1.0, 8.0.2 Oct 6, 2021
Co-authored-by: David Lord <davidism@gmail.com>
@davidism davidism changed the base branch from main to 8.0.x October 8, 2021 21:41
@davidism davidism merged commit 7a226b3 into pallets:8.0.x Oct 8, 2021
@ikapelyukhin ikapelyukhin deleted the fix-multiple-default-values branch October 9, 2021 03:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't set the default value for multiple options with optional values
2 participants