Skip to content

Fix default values of multiple options with optional values #2004

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.

Sign up for GitHub

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

Jump to bottom
Merged
davidism merged 1 commit into from
Oct 8, 2021
Merged

Fix default values of multiple options with optional values #2004

davidism merged 1 commit into from
Oct 8, 2021

Conversation

@ikapelyukhin
Copy link
Contributor

@ikapelyukhin ikapelyukhin commented Jul 13, 2021

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 ikapelyukhin force-pushed the fix-multiple-default-values branch from c9e79f3 to 29c74c8 Compare July 13, 2021 20:28
@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.

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

@davidism
Copy link
Member

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

@ikapelyukhin
Copy link
Contributor Author

ikapelyukhin commented Jul 13, 2021 •
edited
Loading

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 ikapelyukhin mentioned this pull request Aug 3, 2021
Closed
@ikapelyukhin ikapelyukhin force-pushed the fix-multiple-default-values branch from 29c74c8 to 30b704c Compare August 4, 2021 19:13
@ikapelyukhin
Copy link
Contributor Author

ikapelyukhin commented Aug 4, 2021

@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
@ikapelyukhin @davidism
option with multiple and flag_value replaces internal placeholder
aa6f360 
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 force-pushed the fix-multiple-default-values branch from e0c8441 to aa6f360 Compare 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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

8.0.2

Development

Successfully merging this pull request may close these issues.

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

2 participants

@ikapelyukhin @davidism