Skip to content
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.

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

Fix eagerness of help option generated by help_option_names #2811

Merged
merged 2 commits into from
Nov 30, 2024

Conversation

kdeldycke
Copy link
Contributor

@kdeldycke kdeldycke commented Nov 24, 2024

When a help option is generated by the use of help_option_names setting, it loose its eagerness. This issue has an effect on the order in which parameters (and their callbacks) are processed, and breaks the evaluation order described in the documentation: https://click.palletsprojects.com/en/stable/advanced/#callback-evaluation-order

That's because the utility method iter_params_for_processing() is comparing option objects, and the help option is generated on every get_help_option() or get_params(ctx) call. This behavior is demonstrated by a unit-test in this PR.

My solution to fix this issue consist in caching locally the auto-generated help option. This solves the issue.

I used this oportunity to clarify some documentation, and unit-test the main method responsible for the ordering of parameters.

This is more or less a follow up of #2563, which has been merged in the upcoming v8.1.8 release.

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.

@kdeldycke kdeldycke changed the title Unittest parameter eagerness sorting Fix eagerness of help option generated by help_option_names Nov 28, 2024
@kdeldycke kdeldycke force-pushed the eagerness-sorting-unittest branch from 510b516 to 9a6b0cd Compare November 28, 2024 05:06
kdeldycke added a commit to kdeldycke/click-extra that referenced this pull request Nov 28, 2024
src/click/core.py Outdated Show resolved Hide resolved
@kdeldycke kdeldycke force-pushed the eagerness-sorting-unittest branch from 9a6b0cd to 72b0caa Compare November 29, 2024 13:04
This enforce respect of its eagerness
@kdeldycke kdeldycke force-pushed the eagerness-sorting-unittest branch from 72b0caa to 6ee7f05 Compare November 29, 2024 13:31
@kdeldycke kdeldycke changed the base branch from main to stable November 29, 2024 13:33
@kdeldycke
Copy link
Contributor Author

Just fixed all the remaining issues.

I was just wondering why get_help_option() couldn't be replaced by a @cached_property like that:

    @cached_property
    def help_option(self) -> t.Optional["Option"]:
        ctx = self.get_current_context()
        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
        
        return HelpOption(help_options)

    def get_help_option(self, ctx: Context) -> t.Optional["Option"]:
        return self.help_option

But I guess there are some edge-cases regarding the context management. And I don't want to propose refactors that are too deep just as 8.1.8 is around the corner and the 8.2.0 is coming soon after.

Anyway, this PR is ready to be merged upstream for the 8.1.8 release.

@AndreasBackx AndreasBackx merged commit 70c673d into pallets:stable Nov 30, 2024
12 checks passed
@AndreasBackx
Copy link
Collaborator

AndreasBackx commented Nov 30, 2024

Thanks for the fix!

We can look at cached properties in the future perhaps. I'm not bothered by a simple if None.

@AndreasBackx AndreasBackx added this to the 8.1.8 milestone Nov 30, 2024
@AndreasBackx AndreasBackx mentioned this pull request Nov 30, 2024
10 tasks
@kdeldycke kdeldycke deleted the eagerness-sorting-unittest branch November 30, 2024 13:42
@kdeldycke
Copy link
Contributor Author

Thanks @AndreasBackx for the merge!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants