From 6ee7f05c8c28c6c0033ef9849cc724210d155409 Mon Sep 17 00:00:00 2001 From: Kevin Deldycke Date: Fri, 29 Nov 2024 17:31:39 +0400 Subject: [PATCH 1/2] Cache help option generated by help_option_names This enforce respect of its eagerness --- CHANGES.rst | 3 + src/click/core.py | 29 +++++++-- tests/test_commands.py | 132 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index df4e5c79a..22b93f8da 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -18,6 +18,9 @@ Unreleased forced on Windows. :issue:`2606`. - More robust bash version check, fixing problem on Windows with git-bash. :issue:`2638` +- Cache the help option generated by the ``help_option_names`` setting to + respect its eagerness. :pr:`2811` + Version 8.1.7 ------------- diff --git a/src/click/core.py b/src/click/core.py index c623764ee..5845c49da 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -116,9 +116,16 @@ def iter_params_for_processing( invocation_order: t.Sequence["Parameter"], declaration_order: t.Sequence["Parameter"], ) -> t.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") -> t.Tuple[bool, float]: @@ -1223,6 +1230,7 @@ def __init__( self.options_metavar = options_metavar self.short_help = short_help self.add_help_option = add_help_option + self._help_option: HelpOption | None = None self.no_args_is_help = no_args_is_help self.hidden = hidden self.deprecated = deprecated @@ -1288,16 +1296,25 @@ def get_help_option(self, ctx: Context) -> t.Optional["Option"]: """Returns the help option object. Unless ``add_help_option`` is ``False``. + + .. versionchanged:: 8.1.8 + 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: return None - # Avoid circular import. - from .decorators import HelpOption + # 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): + # Avoid circular import. + from .decorators import HelpOption + self._help_option = HelpOption(help_options) - return HelpOption(help_options) + 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 ed9d96f3c..dcf66acef 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -305,6 +305,138 @@ 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 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 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 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 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 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 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) From ab270b4617a4a743aecce51442cb5613d4aa33f4 Mon Sep 17 00:00:00 2001 From: Andreas Backx Date: Sat, 30 Nov 2024 00:43:14 +0000 Subject: [PATCH 2/2] Fix old style typing and remove getattr. --- src/click/core.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/click/core.py b/src/click/core.py index 5845c49da..e6305011a 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -40,6 +40,7 @@ if t.TYPE_CHECKING: import typing_extensions as te + from .decorators import HelpOption from .shell_completion import CompletionItem F = t.TypeVar("F", bound=t.Callable[..., t.Any]) @@ -1230,7 +1231,7 @@ def __init__( self.options_metavar = options_metavar self.short_help = short_help self.add_help_option = add_help_option - self._help_option: HelpOption | None = None + self._help_option: t.Optional[HelpOption] = None self.no_args_is_help = no_args_is_help self.hidden = hidden self.deprecated = deprecated @@ -1309,9 +1310,10 @@ def get_help_option(self, ctx: Context) -> t.Optional["Option"]: # 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): + if self._help_option is None: # Avoid circular import. from .decorators import HelpOption + self._help_option = HelpOption(help_options) return self._help_option