diff --git a/CHANGES.rst b/CHANGES.rst index a90ff58f1..97701f546 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -78,6 +78,9 @@ Unreleased - ``echo_via_pager`` will not ignore ``KeyboardInterrupt`` anymore. This allows the user to search for future output of the generator when using less and then aborting the program using ctrl-c. +- Cache the help option generated by the ``help_option_names`` setting to + respect its eagerness. :pr:`2811` + - ``deprecated: bool | str`` can now be used on options and arguments. This previously was only available for ``Command``. The message can now also be diff --git a/src/click/core.py b/src/click/core.py index 1c1a46714..a30fc7d57 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -116,9 +116,16 @@ def iter_params_for_processing( invocation_order: cabc.Sequence[Parameter], declaration_order: cabc.Sequence[Parameter], ) -> list[Parameter]: - """Given a sequence of parameters in the order as should be considered - for processing and an iterable of parameters that exist, this returns - a list in the correct order as they should be processed. + """Returns all declared parameters in the order they should be processed. + + The declared parameters are re-shuffled depending on the order in which + they were invoked, as well as the eagerness of each parameters. + + The invocation order takes precedence over the declaration order. I.e. the + order in which the user provided them to the CLI is respected. + + This behavior and its effect on callback evaluation is detailed at: + https://click.palletsprojects.com/en/stable/advanced/#callback-evaluation-order """ def sort_key(item: Parameter) -> tuple[bool, float]: @@ -1014,7 +1021,11 @@ def get_help_option_names(self, ctx: Context) -> list[str]: return list(all_names) def get_help_option(self, ctx: Context) -> Option | None: - """Returns the help option object.""" + """Returns the help option object. + + .. versionchanged:: 8.2.0 + The help option is now cached to avoid creating it multiple times. + """ help_options = self.get_help_option_names(ctx) if not help_options or not self.add_help_option: @@ -1025,14 +1036,21 @@ def show_help(ctx: Context, param: Parameter, value: str) -> None: echo(ctx.get_help(), color=ctx.color) ctx.exit() - return Option( - help_options, - is_flag=True, - is_eager=True, - expose_value=False, - callback=show_help, - help=_("Show this message and exit."), - ) + # Cache the help option object in private _help_option attribute to + # avoid creating it multiple times. Not doing this will break the + # callback odering by iter_params_for_processing(), which relies on + # object comparison. + if not getattr(self, "_help_option", None): + self._help_option = Option( + help_options, + is_flag=True, + is_eager=True, + expose_value=False, + callback=show_help, + help=_("Show this message and exit."), + ) + + return self._help_option def make_parser(self, ctx: Context) -> _OptionParser: """Creates the underlying option parser for this command.""" diff --git a/tests/test_commands.py b/tests/test_commands.py index eb8df85f9..0a31eaa71 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -301,6 +301,144 @@ def test_group_add_command_name(runner): assert result.exit_code == 0 +@pytest.mark.parametrize( + ("invocation_order", "declaration_order", "expected_order"), + [ + # Non-eager options. + ([], ["-a"], ["-a"]), + (["-a"], ["-a"], ["-a"]), + ([], ["-a", "-c"], ["-a", "-c"]), + (["-a"], ["-a", "-c"], ["-a", "-c"]), + (["-c"], ["-a", "-c"], ["-c", "-a"]), + ([], ["-c", "-a"], ["-c", "-a"]), + (["-a"], ["-c", "-a"], ["-a", "-c"]), + (["-c"], ["-c", "-a"], ["-c", "-a"]), + (["-a", "-c"], ["-a", "-c"], ["-a", "-c"]), + (["-c", "-a"], ["-a", "-c"], ["-c", "-a"]), + # Eager options. + ([], ["-b"], ["-b"]), + (["-b"], ["-b"], ["-b"]), + ([], ["-b", "-d"], ["-b", "-d"]), + (["-b"], ["-b", "-d"], ["-b", "-d"]), + (["-d"], ["-b", "-d"], ["-d", "-b"]), + ([], ["-d", "-b"], ["-d", "-b"]), + (["-b"], ["-d", "-b"], ["-b", "-d"]), + (["-d"], ["-d", "-b"], ["-d", "-b"]), + (["-b", "-d"], ["-b", "-d"], ["-b", "-d"]), + (["-d", "-b"], ["-b", "-d"], ["-d", "-b"]), + # Mixed options. + ([], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + (["-a"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + (["-b"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + (["-c"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-c", "-a"]), + (["-d"], ["-a", "-b", "-c", "-d"], ["-d", "-b", "-a", "-c"]), + (["-a", "-b"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + (["-b", "-a"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + (["-d", "-c"], ["-a", "-b", "-c", "-d"], ["-d", "-b", "-c", "-a"]), + (["-c", "-d"], ["-a", "-b", "-c", "-d"], ["-d", "-b", "-c", "-a"]), + (["-a", "-b", "-c", "-d"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + (["-b", "-d", "-a", "-c"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]), + ([], ["-b", "-d", "-e", "-a", "-c"], ["-b", "-d", "-e", "-a", "-c"]), + (["-a", "-d"], ["-b", "-d", "-e", "-a", "-c"], ["-d", "-b", "-e", "-a", "-c"]), + (["-c", "-d"], ["-b", "-d", "-e", "-a", "-c"], ["-d", "-b", "-e", "-c", "-a"]), + ], +) +def test_iter_params_for_processing( + invocation_order, declaration_order, expected_order +): + parameters = { + "-a": click.Option(["-a"]), + "-b": click.Option(["-b"], is_eager=True), + "-c": click.Option(["-c"]), + "-d": click.Option(["-d"], is_eager=True), + "-e": click.Option(["-e"], is_eager=True), + } + + invocation_params = [parameters[opt_id] for opt_id in invocation_order] + declaration_params = [parameters[opt_id] for opt_id in declaration_order] + expected_params = [parameters[opt_id] for opt_id in expected_order] + + assert ( + click.core.iter_params_for_processing(invocation_params, declaration_params) + == expected_params + ) + + +def test_help_param_priority(runner): + """Cover the edge-case in which the eagerness of help option was not + respected, because it was internally generated multiple times. + + See: https://github.com/pallets/click/pull/2811 + """ + + def print_and_exit(ctx, param, value): + if value: + click.echo(f"Value of {param.name} is: {value}") + ctx.exit() + + @click.command(context_settings={"help_option_names": ("--my-help",)}) + @click.option("-a", is_flag=True, expose_value=False, callback=print_and_exit) + @click.option( + "-b", is_flag=True, expose_value=False, callback=print_and_exit, is_eager=True + ) + def cli(): + pass + + # --my-help is properly called and stop execution. + result = runner.invoke(cli, ["--my-help"]) + assert "Value of a is: True" not in result.stdout + assert "Value of b is: True" not in result.stdout + assert "--my-help" in result.stdout + assert not result.stderr + assert result.exit_code == 0 + + # -a is properly called and stop execution. + result = runner.invoke(cli, ["-a"]) + assert "Value of a is: True" in result.stdout + assert "Value of b is: True" not in result.stdout + assert "--my-help" not in result.stdout + assert not result.stderr + assert result.exit_code == 0 + + # -a takes precedence over -b and stop execution. + result = runner.invoke(cli, ["-a", "-b"]) + assert "Value of a is: True" not in result.stdout + assert "Value of b is: True" in result.stdout + assert "--my-help" not in result.stdout + assert not result.stderr + assert result.exit_code == 0 + + # --my-help is eager by default so takes precedence over -a and stop + # execution, whatever the order. + for args in [["-a", "--my-help"], ["--my-help", "-a"]]: + result = runner.invoke(cli, args) + assert "Value of a is: True" not in result.stdout + assert "Value of b is: True" not in result.stdout + assert "--my-help" in result.stdout + assert not result.stderr + assert result.exit_code == 0 + + # Both -b and --my-help are eager so they're called in the order they're + # invoked by the user. + result = runner.invoke(cli, ["-b", "--my-help"]) + assert "Value of a is: True" not in result.stdout + assert "Value of b is: True" in result.stdout + assert "--my-help" not in result.stdout + assert not result.stderr + assert result.exit_code == 0 + + # But there was a bug when --my-help is called before -b, because the + # --my-help option created by click via help_option_names is internally + # created twice and is not the same object, breaking the priority order + # produced by iter_params_for_processing. + result = runner.invoke(cli, ["--my-help", "-b"]) + assert "Value of a is: True" not in result.stdout + assert "Value of b is: True" not in result.stdout + assert "--my-help" in result.stdout + assert not result.stderr + assert result.exit_code == 0 + + def test_unprocessed_options(runner): @click.command(context_settings=dict(ignore_unknown_options=True)) @click.argument("args", nargs=-1, type=click.UNPROCESSED)