diff --git a/CHANGES.rst b/CHANGES.rst index 95395eb28..95d6556df 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -9,6 +9,8 @@ Unreleased :pr:`3055` - Replace ``Sentinel.UNSET`` default values by ``None`` as they're passed through the ``Context.invoke()`` method. :issue:`3066` :issue:`3065` :pr:`3068` +- Fix conversion of ``Sentinel.UNSET`` happening too early, which caused incorrect + behavior for multiple parameters using the same name. :issue:`3071` :pr:`3079` Version 8.3.0 -------------- diff --git a/src/click/core.py b/src/click/core.py index cd8052f7d..5f4728717 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -1226,6 +1226,19 @@ def parse_args(self, ctx: Context, args: list[str]) -> list[str]: for param in iter_params_for_processing(param_order, self.get_params(ctx)): _, args = param.handle_parse_result(ctx, opts, args) + # We now have all parameters' values into `ctx.params`, but the data may contain + # the `UNSET` sentinel. + # Convert `UNSET` to `None` to ensure that the user doesn't see `UNSET`. + # + # Waiting until after the initial parse to convert allows us to treat `UNSET` + # more like a missing value when multiple params use the same name. + # Refs: + # https://github.com/pallets/click/issues/3071 + # https://github.com/pallets/click/pull/3079 + for name, value in ctx.params.items(): + if value is UNSET: + ctx.params[name] = None + if args and not ctx.allow_extra_args and not ctx.resilient_parsing: ctx.fail( ngettext( @@ -2538,17 +2551,14 @@ def handle_parse_result( # We skip adding the value if it was previously set by another parameter # targeting the same variable name. This prevents parameters competing for # the same name to override each other. - and self.name not in ctx.params + and (self.name not in ctx.params or ctx.params[self.name] is UNSET) ): # Click is logically enforcing that the name is None if the parameter is # not to be exposed. We still assert it here to please the type checker. assert self.name is not None, ( f"{self!r} parameter's name should not be None when exposing value." ) - # Normalize UNSET values to None, as we're about to pass them to the - # command function and move them to the pure-Python realm of user-written - # code. - ctx.params[self.name] = value if value is not UNSET else None + ctx.params[self.name] = value return value, args diff --git a/tests/test_defaults.py b/tests/test_defaults.py index 3f293e881..31eee844b 100644 --- a/tests/test_defaults.py +++ b/tests/test_defaults.py @@ -84,3 +84,29 @@ def foo(name): result = runner.invoke(cli, ["foo", "--help"], default_map={"foo": {"name": True}}) assert "default: name" in result.output + + +def test_shared_param_prefers_first_default(runner): + """test that the first default is chosen when multiple flags share a param name""" + + @click.command + @click.option("--red", "color", flag_value="red") + @click.option("--green", "color", flag_value="green", default=True) + def prefers_green(color): + click.echo(color) + + @click.command + @click.option("--red", "color", flag_value="red", default=True) + @click.option("--green", "color", flag_value="green") + def prefers_red(color): + click.echo(color) + + result = runner.invoke(prefers_green, []) + assert "green" in result.output + result = runner.invoke(prefers_green, ["--red"]) + assert "red" in result.output + + result = runner.invoke(prefers_red, []) + assert "red" in result.output + result = runner.invoke(prefers_red, ["--green"]) + assert "green" in result.output