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.

Sign up for GitHub

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

Jump to bottom

Move --help option defaults from its class to its decorator #2840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Move --help option defaults from its class to its decorator #2840

wants to merge 1 commit into from

Conversation

kdeldycke
Copy link
Contributor

@kdeldycke kdeldycke commented Jan 9, 2025 •
edited
Loading

This is a proposal to fix #2832, which points to a subtle change in behavior of the help option introduced in 8.1.8.

The root of the problem is that by deduplicating code in 8.1.8 (see: #2563), I introduced a HelpOption class, to centralize the setup necessary to make a standard Option behave like an help option.

In #2832, a user is relying on a custom class to feed the @help_option decorator. This bypass all the custom setup made by HelpOption, and produce the discrepancy in behavior between 8.1.7 and 8.1.8.

This PR:

  • Reverts back to setting up all parameters of the help option in the decorator instead of the HelpOption class
  • Aligns @help_option implementation with the way Click is doing for the others (like @password_option, @version_option, ...)
  • Removes the click.decorators.HelpOption class as a consequence
  • Adds a complete unit test to check combination of custom class and option names leads to expected results

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 mentioned this pull request Jan 9, 2025
Open
@kdeldycke kdeldycke changed the title Allow customization of help option right from its decorator Allow customization of help option from its decorator Jan 9, 2025
@kdeldycke kdeldycke changed the title Allow customization of help option from its decorator Move --help option defaults from its class to its decorator Jan 9, 2025
@kdeldycke
Allow customization of help option right from its decorator.
34d2533 
Removes `click.decorators.HelpOption` class.

Closes pallets#2832.
kdeldycke added a commit to kdeldycke/click-extra that referenced this pull request Jan 10, 2025
@kdeldycke
Set --help/-h as defaults for @help_option decorator
b77007d 
Remove custom HelpOption class.

Refs:
pallets/click#2563
pallets/click#2832
pallets/click#2840
@AndreasBackx
Copy link
Collaborator

AndreasBackx commented Jan 12, 2025 •
edited
Loading

Can you rebase this on stable then we can include this in 8.1.9?

I assume we can keep this in 8.2.0, though we'll need to document it.

@kdeldycke
Copy link
Contributor Author

Yes! I can rebase it to 8.1.9. I was targeting 8.2.0 as I didn't know if a 8.1.9 would see the light of day given that 8.2.0 seems imminent.

Also, something I realized, when #2563 was merged back into main branch (i.e. the future 8.2.0 release), the new HelpOption class was properly merged, but the duplicate in-place Option creation in get_help_option() was not replaced with the new HelpOption. See:

click/src/click/core.py

Lines 1028 to 1035 in 8fcf9b0

return Option(
help_options,
is_flag=True,
is_eager=True,
expose_value=False,
callback=show_help,
help=_("Show this message and exit."),
)

So I felt like this PR, by targeting main could solve both issues at the same time. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression: Help option subclassing no longer works with verion 8.1.8
2 participants
@kdeldycke @AndreasBackx