Differentiate absence of value and value of absence for default and flag_value arguments

[kdeldycke]

Tl;Dr: this PR makes user-provided values (like None) first-class citizens.

Description

• Introduce the UNSET sentinel value to track when default and flag_value have been explicitly set by the user on Parameter, Option and Argument
• Allow None or any other Python value to be used as default and flag_value
• Fix dozens of flag-adjacent edge-cases
• Adds a new class to create self-documenting sentinel objects
• Adds comments and docstrings for internal methods (this should help write better higher-level documentation and tutorial in Sphinx)
• Rename some obscure internal variables to make code more approachable to external contributors
• Adds 200+ unittests
• Streamline code
• Fix typos
• Old-school hand-made code, LLMs only used as a glorified auto-complete

Context

The pandora box was open in Click 8.2.0 with an attempt to always returns the flag_value if an option gets activated by its environment variable:

• flag_value is not taken into account with envvar #2746
• Added functionality to set flag_value correctly when using envvar #2788

Another patch in Click 8.2.0 tried to fix how flag_value gets aligned to its default:

• Only try to set flag_value if is_flag is true #2829

The effect of these patches were reported after 8.2.0 release, and uncovered how tangled the flag logic is regarding its default, flag_value, type and envvar parameters:

• Test fails with click 8.2.0 if both "type=bool" and "default=False" are specified #2863
• Click 8.2.0 ignores is_flag options that have a type #2894
• Boolean flags are always False #2897
• Flag value is always true when setting any envvar #2952

A first attempt to fix these issues was made in Click 8.2.1:

• Fix flag value set when is_flag=True and type is provided #2930

As I was aware of some of these edge-cases in 2023, I tackled this challenge and made a big PR to reconcile everything:

• Fix reconciliation of envvar with default, flag_value and type parameters for flag options #2956

This was released as Click 8.2.2. But it was not enough and even more nastier edge-cases were spotted (see the Progress section below). 8.2.2 was finally yanked to prevent widespread frustration.

The situation cannot be fixed with trivial changes as we are confronted with internal design limits. This PR is trying to fix all this mess.

Progress

Currently playing Whac-A-Mole, but ultimate goal is to fix the following issues:

• Binary flags vs default=None: semantics and issue with prompt #1992

  Status: ✅ 100% fixed

  Proof:

  • click/tests/test_termui.py

    Lines 492 to 554 in 14e6cee

    	BOOLEAN_FLAG_PROMPT_CASES = [
    	###
    	### Test cases with prompt=True explicitly enabled for the flag.
    	###
    	# Prompt is allowed and the flag has no default, so it prompts.
    	({"prompt": True}, [], "[y/N]", "y", True),
    	({"prompt": True}, [], "[y/N]", "n", False),
    	# Empty input default to False.
    	({"prompt": True}, [], "[y/N]", "", False),
    	# Changing the default to True, makes the prompt change to [Y/n].
    	({"prompt": True, "default": True}, [], "[Y/n]", "", True),
    	({"prompt": True, "default": True}, [], "[Y/n]", "y", True),
    	({"prompt": True, "default": True}, [], "[Y/n]", "n", False),
    	# False is the default's default, so it prompts with [y/N].
    	({"prompt": True, "default": False}, [], "[y/N]", "", False),
    	({"prompt": True, "default": False}, [], "[y/N]", "y", True),
    	({"prompt": True, "default": False}, [], "[y/N]", "n", False),
    	# Defaulting to None, prompts with [y/n], which makes the user explicitly choose
    	# between True or False.
    	({"prompt": True, "default": None}, [], "[y/n]", "y", True),
    	({"prompt": True, "default": None}, [], "[y/n]", "n", False),
    	# Random string default is treated as a truthy value, so it prompts with [Y/n].
    	({"prompt": True, "default": "foo"}, [], "[Y/n]", "", True),
    	({"prompt": True, "default": "foo"}, [], "[Y/n]", "y", True),
    	({"prompt": True, "default": "foo"}, [], "[Y/n]", "n", False),
    	###
    	### Test cases with required=True explicitly enabled for the flag.
    	###
    	# A required flag just raises an error unless a default is set.
    	({"required": True}, [], None, None, MissingParameter),
    	({"required": True, "default": True}, [], None, None, True),
    	({"required": True, "default": False}, [], None, None, False),
    	({"required": True, "default": None}, [], None, None, None),
    	({"required": True, "default": "on"}, [], None, None, True),
    	({"required": True, "default": "off"}, [], None, None, False),
    	({"required": True, "default": "foo"}, [], None, None, BadParameter),
    	###
    	### Explicitly passing the flag to the CLI bypass any prompt, whatever the
    	### configuration of the flag.
    	###
    	# Flag allowing a prompt.
    	({"prompt": True}, ["--flag"], None, None, True),
    	({"prompt": True}, ["--no-flag"], None, None, False),
    	({"prompt": True, "default": None}, ["--flag"], None, None, True),
    	({"prompt": True, "default": None}, ["--no-flag"], None, None, False),
    	({"prompt": True, "default": True}, ["--flag"], None, None, True),
    	({"prompt": True, "default": True}, ["--no-flag"], None, None, False),
    	({"prompt": True, "default": False}, ["--flag"], None, None, True),
    	({"prompt": True, "default": False}, ["--no-flag"], None, None, False),
    	({"prompt": True, "default": "foo"}, ["--flag"], None, None, True),
    	({"prompt": True, "default": "foo"}, ["--no-flag"], None, None, False),
    	# Required flag.
    	({"required": True}, ["--flag"], None, None, True),
    	({"required": True}, ["--no-flag"], None, None, False),
    	({"required": True, "default": None}, ["--flag"], None, None, True),
    	({"required": True, "default": None}, ["--no-flag"], None, None, False),
    	({"required": True, "default": True}, ["--flag"], None, None, True),
    	({"required": True, "default": True}, ["--no-flag"], None, None, False),
    	({"required": True, "default": False}, ["--flag"], None, None, True),
    	({"required": True, "default": False}, ["--no-flag"], None, None, False),
    	({"required": True, "default": "foo"}, ["--flag"], None, None, True),
    	({"required": True, "default": "foo"}, ["--no-flag"], None, None, False),
    	]

• flag_value is passed through ParamType.convert #2012

  Status: ✅ 100% fixed

  Proof:

  • click/tests/test_options.py

    Lines 1749 to 1817 in da07421

    	class Class1:
    	pass
    	class Class2:
    	pass
    	@pytest.mark.parametrize(
    	("opt_params", "default", "args", "expected"),
    	[
    	# Default type fallback to string.
    	({}, True, [], "True"),
    	({}, True, ["--cls1"], "<class 'test_options.Class1'>"),
    	({}, True, ["--cls2"], "<class 'test_options.Class2'>"),
    	# Even the default is processed as a string.
    	({}, "True", [], "True"),
    	({}, None, [], None),
    	# To get the classes as-is, we need to specify the type as UNPROCESSED.
    	({"type": click.UNPROCESSED}, True, [], True),
    	({"type": click.UNPROCESSED}, True, ["--cls1"], Class1),
    	({"type": click.UNPROCESSED}, True, ["--cls2"], Class2),
    	# Setting the default to a class, an instance of the class is returned instead
    	# of the class itself, because the default is allowed to be callable (and
    	# consummd). And this what happens the type is.
    	(
    	{},
    	Class1,
    	[],
    	re.compile(r"'<test_options.Class1 object at 0x[0-9A-Fa-f]+>'"),
    	),
    	(
    	{},
    	Class2,
    	[],
    	re.compile(r"'<test_options.Class2 object at 0x[0-9A-Fa-f]+>'"),
    	),
    	(
    	{"type": click.UNPROCESSED},
    	Class1,
    	[],
    	re.compile(r"<test_options.Class1 object at 0x[0-9A-Fa-f]+>"),
    	),
    	(
    	{"type": click.UNPROCESSED},
    	Class2,
    	[],
    	re.compile(r"<test_options.Class2 object at 0x[0-9A-Fa-f]+>"),
    	),
    	],
    	)
    	def test_custom_type_python_class_flag_value(
    	runner, opt_params, default, args, expected
    	):
    	"""A reproduction of https://github.com/pallets/click/issues/2012"""
    	@click.command()
    	@click.option(
    	"--cls1", "config_cls", flag_value=Class1, **opt_params, default=default
    	)
    	@click.option("--cls2", "config_cls", flag_value=Class2, **opt_params)
    	def cli(config_cls):
    	click.echo(repr(config_cls), nl=False)
    	result = runner.invoke(cli, args)
    	if isinstance(expected, re.Pattern):
    	assert re.match(expected, result.output)
    	else:
    	assert result.output == repr(expected)

  • click/tests/test_options.py

    Lines 1850 to 1864 in f9d006e

    	###
    	### Reproduces https://github.com/pallets/click/issues/2012#issuecomment-892437060
    	###
    	(
    	{"flag_value": 1, "default": True},
    	{"flag_value": "1"},
    	[],
    	1,
    	),
    	(
    	{"flag_value": 1, "type": int, "default": True},
    	{"flag_value": "1", "type": int},
    	[],
    	1,
    	),

  • click/tests/test_options.py

    Lines 1727 to 1744 in f9d006e

    	# Reproduction of cases from
    	# https://github.com/pallets/click/issues/2012#issuecomment-892437060
    	(
    	{"flag_value": EngineType.OSS, "default": True},
    	[],
    	"True",
    	),
    	(
    	{"type": EngineType, "flag_value": EngineType.OSS, "default": True},
    	[],
    	EngineType.OSS,
    	),
    	({"flag_value": 1, "default": True}, [], 1),
    	({"flag_value": "1", "default": True}, [], "True"),
    	({"flag_value": 1, "type": str, "default": True}, [], "True"),
    	({"flag_value": "1", "type": str, "default": True}, [], "True"),
    	({"flag_value": 1, "type": int, "default": True}, [], 1),
    	({"flag_value": "1", "type": int, "default": True}, [], 1),

• Support default for arguments with multiple values #2164

  Status: ✅ 100% fixed

  Proof:

  • click/tests/test_arguments.py

    Lines 388 to 392 in 3bd9e22

    	# Unbounded arguments are allowed to have a default.
    	# See: https://github.com/pallets/click/issues/2164
    	[{"nargs": -1, "default": [42]}, [], (42,)],
    	[{"nargs": -1, "default": None}, [], ()],
    	[{"nargs": -1, "default": {1, 2, 3, 4, 5}}, [], (1, 2, 3, 4, 5)],

• make flag_value=None actually set flag value to None #2514

  Status: ✅ 100% fixed

  Proof:

  • click/tests/test_options.py

    Lines 1278 to 1279 in 14e6cee

    	({"type": bool, "flag_value": None}, [], False),
    	({"type": bool, "flag_value": None}, ["--foo"], None),

• The flag_value shadows default in click.option() #2610

  Status: ✅ 100% fixed

  Proof:

  • click/tests/test_options.py

    Lines 1697 to 1718 in 14e6cee

    	def test_custom_type_frozenset_flag_value(runner):
    	"""A reproduction of https://github.com/pallets/click/issues/2610"""
    	@click.command()
    	@click.option(
    	"--without-scm-ignore-files",
    	"scm_ignore_files",
    	is_flag=True,
    	type=frozenset,
    	flag_value=frozenset(),
    	default=frozenset(["git"]),
    	)
    	def rcli(scm_ignore_files):
    	click.echo(repr(scm_ignore_files), nl=False)
    	result = runner.invoke(rcli)
    	assert result.stdout == "frozenset({'git'})"
    	assert result.exit_code == 0
    	result = runner.invoke(rcli, ["--without-scm-ignore-files"])
    	assert result.stdout == "frozenset()"
    	assert result.exit_code == 0

  • Add support for set and frozenset (and dict?) as native Click types #3036

• Click 8.2.2 breaks auto-reloader-active on flask --debug #3024

  Status: ✅ 100% fixed

  Proof:

  • click/tests/test_options.py

    Lines 1254 to 1255 in 14e6cee

    	({"type": bool, "default": None}, [], None),
    	({"type": bool, "default": None}, ["--foo"], True),

  • click/tests/test_options.py

    Lines 1656 to 1694 in 14e6cee

    	class EngineType(enum.Enum):
    	OSS = enum.auto()
    	PRO = enum.auto()
    	MAX = enum.auto()
    	@pytest.mark.parametrize(
    	("opt_params", "args", "expected"),
    	[
    	({"flag_value": None}, [], "None"),
    	({"flag_value": None}, ["--pro"], "None"),
    	({"flag_value": EngineType.MAX}, [], "None"),
    	({"flag_value": EngineType.MAX}, ["--pro"], "<EngineType.MAX: 3>"),
    	({"default": EngineType.OSS}, [], "<EngineType.OSS: 1>"),
    	(
    	{"default": EngineType.OSS},
    	["--pro"],
    	"Error: Option '--pro' requires an argument.\n",
    	),
    	({"is_flag": True, "default": EngineType.OSS}, [], "<EngineType.OSS: 1>"),
    	(
    	{"is_flag": True, "default": EngineType.OSS},
    	["--pro"],
    	"<EngineType.OSS: 1>",
    	),
    	],
    	)
    	def test_custom_type_enum_flag_value(runner, opt_params, args, expected):
    	"""A reproduction of
    	https://github.com/pallets/click/issues/3024#issuecomment-3146480714
    	"""
    	@click.command()
    	@click.option("--pro", type=EngineType, **opt_params)
    	def scan(pro):
    	click.echo(repr(pro), nl=False)
    	result = runner.invoke(scan, args)
    	assert result.output == expected

  • click/tests/test_options.py

    Lines 1697 to 1722 in 9fcd409

    	def test_custom_type_click_class_flag_value(runner):
    	"""A reproduction of
    	https://github.com/pallets/click/issues/3024#issuecomment-3146511356
    	"""
    	NO_CONFIG = object()
    	class ConfigParamType(click.ParamType):
    	name = "config"
    	def convert(self, value, param, ctx):
    	if value is NO_CONFIG:
    	return value
    	else:
    	return click.Path(exists=True, dir_okay=False).convert(
    	value, param, ctx
    	)
    	@click.command()
    	@click.option("-c", "--config", type=ConfigParamType())
    	@click.option("--no-config", "config", flag_value=NO_CONFIG, type=ConfigParamType())
    	def main(config):
    	click.echo(repr(config), nl=False)
    	result = runner.invoke(main)
    	assert result.output == repr(None)

  • click/tests/test_basic.py

    Lines 279 to 297 in 373429e

    	@pytest.mark.parametrize(
    	("default", "args", "expect"),
    	(
    	# See: https://github.com/pallets/click/issues/3024#issuecomment-3146199461
    	(True, ["--upper"], "upper"),
    	(True, ["--lower"], "lower"),
    	(True, [], "True"),
    	("upper", [], "upper"),
    	),
    	)
    	def test_flag_value(runner, default, args, expect):
    	@click.command()
    	@click.option("--upper", "transformation", flag_value="upper", default=default)
    	@click.option("--lower", "transformation", flag_value="lower")
    	def cli(transformation):
    	click.echo(repr(transformation), nl=False)
    	result = runner.invoke(cli, args)
    	assert result.output == repr(expect)

  • click/docs/options.md

    Lines 317 to 329 in 373429e

    	To have an flag pass a value to the underlying function set `flag_value`. This automatically sets `is_flag=True`. To influence the value-less behavior, force its value with `default='upper'`. Setting flag values can be used to create patterns like this:
    	```{eval-rst}
    	.. click:example::
    	import sys
    	@click.command()
    	@click.option('--upper', 'transformation', flag_value='upper', default='upper')
    	@click.option('--lower', 'transformation', flag_value='lower')
    	def info(transformation):
    	click.echo(getattr(sys.platform, transformation)())

  • click/tests/test_options.py

    Lines 1725 to 1746 in b6db38e

    	@pytest.mark.parametrize(
    	("args", "expected"),
    	[
    	([], None),
    	(["--oss"], EngineType.OSS),
    	(["--pro"], EngineType.PRO),
    	],
    	)
    	def test_unprocessed_type_enum_flag_value_shared_valiable(runner, args, expected):
    	"""A reproduction of
    	https://github.com/pallets/click/issues/3024#issuecomment-3146508536
    	"""
    	@click.command()
    	@click.option("--oss", "mode", flag_value=EngineType.OSS, type=click.UNPROCESSED)
    	@click.option("--pro", "mode", flag_value=EngineType.PRO, type=click.UNPROCESSED)
    	def main(mode):
    	click.echo(repr(mode), nl=False)
    	result = runner.invoke(main, args)
    	assert result.output == repr(expected)
    	assert result.exit_code == 0

[Rowlando13]

From just a quick look at this, it is pretty cool!

[kdeldycke]

From just a quick look at this, it is pretty cool!

I solved 99% of issues and all tests are passing. I am now trying to hunt for even more obscure edge-cases and adding more tests.

So I'm going to polish it in the next couple of days before requesting a proper review. In the meantime, I'm continue hacking on it in the open.

[davidism]

Great work on tracking all this down, been following all week.

[kdeldycke]

Great work on tracking all this down, been following all week.

I was indeed working the full week on that! 😮‍💨

[kdeldycke]

I was trying to figure out exactly what you did to address #2012, and I think you just added tests for what's already happening, didn't change anything? Can you confirm how you addressed it.

I initially covered all the cases discussed over there to highlight your comment that, indeed, due to your changes in the pipeline, these types were fed as string type. In which case using click.UNPROCESSED was the solution. So for me #2012 was solved, as in: "this is now expected in the new Click 8.x pipeline, now use UNPROCESSED". And I just confirmed/locked down the new behavior by unit tests.

But since we re-establish the legacy case of options with flag_value and default=True in 06847da, we reverted back to solving the initial complain of #2012 as it was in Click 7.x.

[kdeldycke]

I just addressed all the comments. I'm ready for a final review.

[Rowlando13]

Looks like there is a docs build issue. I am going to merge then open it in another pr. @davidism I looked through all the comments and they were addressed. If you think they were not, I would suggest opening an issue and we can fix it there. The chain has gotten so long that it is hard to follow. I might have missed one.

[Rowlando13]

@kdeldycke Awesome work!

[kdeldycke]

Arf no don't merge yet! I wanted to squash some commits first! 😂 Well, whatever. I don't mind. ¯\_(ツ)_/¯

[davidism]

Sorry, wasn't ready to merge this yet either. I still want to understand what's going on with flag_value="value", default=True. Why is this considered legacy? There's a lot of strong language in the docs and code now implying that it's barely supported, but as far as I can tell is completely fine. default=True says "use this option's flag value as the default value". What's the issue with this?

I'm also not following the explanation #3030 (comment) of UNPROCESSED and how it relates to this.

[davidism]

@Rowlando13 don't merge huge work commit chains like this in the future. It makes it way harder to use git blame in the future to figure out what the actual intended change was, as now we have to track the change through all the work and review as well. Either squash manually into a few commits, or use the Squash Merge feature in GitHub to create one commit.

[Rowlando13]

@davidism My bad. Will do in the future. I thought we wanted to preserve the process.

[Rowlando13]

@davidism shall I revert the merge?

[davidism]

No it's fine, not a big deal, just something to keep in mind.

[kdeldycke]

Sorry, wasn't ready to merge this yet either. I still want to understand what's going on with flag_value="value", default=True. Why is this considered legacy? There's a lot of strong language in the docs and code now implying that it's barely supported, but as far as I can tell is completely fine. default=True says "use this option's flag value as the default value". What's the issue with this?

Ah yes, sorry for the "strong langage"! 😅 I'm too deep into Click internals to have good judgment about what makes a visible breaking change from a mere behind the scene arrangement. So thanks for the feedbacks you made over the past few weeks (like the :meta private: and so on).

I just made a new PR to not declare default=True as a legacy case: #3049

[kdeldycke]

@Rowlando13 don't merge huge work commit chains like this in the future. It makes it way harder to use git blame in the future to figure out what the actual intended change was, as now we have to track the change through all the work and review as well. Either squash manually into a few commits, or use the Squash Merge feature in GitHub to create one commit.

I guess @davidism and I had a mutual implied understanding that PRs are OK to be messy while we work things up. Once we all agree about the quality, there's always time before merging to clean-up.

For me being messy is part of the process, especially on big changes like this. I am not afraid of doing stupid stuff, making mistakes and backtracking on code. And doing all of that in the open. Writing code that way allow me to gather feedback early. And course-correct if I am not going in the right direction.

Anyway, that is also part of learning to work together. No big deal indeed! I'll make sure to address everything so we can release 8.3.0 together and move on at a more relaxed pace! :)

[mcepl]

Not only I completely agree with @davidism , but I would go much further. Sit down and read http://who-t.blogspot.com/2009/12/on-commit-messages.html ... it is not about letting your dirty laundry hanging around (and nobody is interested in that), it is about creating good foundation for yourself when you study the change a year later.

[kdeldycke]

I'm also not following the explanation #3030 (comment) of UNPROCESSED and how it relates to this.

Hmm. Re-reading #2012 I think I overlooked a detail. Let's re-open it while I investigate.