Skip to content

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

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
Merged
merged 1 commit into from
Mar 18, 2025
Merged

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

merged 1 commit into from
Mar 18, 2025

Conversation

kdeldycke
Copy link
Collaborator

@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
Closed
@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 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
Collaborator 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?

davidism
davidism reviewed Mar 17, 2025
View reviewed changes
davidism
davidism reviewed Mar 17, 2025
View reviewed changes
@davidism
Copy link
Member

Approved with the suggestions I just commented.

@kdeldycke
Allow customization of help option right from its decorator.
0b7a9ac 
Removes `click.decorators.HelpOption` class.

Closes #2832.

# Conflicts:
#	CHANGES.rst
@kdeldycke
Copy link
Collaborator Author

I just merged the changes proposed by @davidism and rebase this PR on top of main.

All tests are passing but the typing workflow job, which is also failing for the same reason on the vanilla main branch. Same thing for the pre-commit.yaml workflow that is failing for reasons not related to this PR.

So unless you have other issues to point to, I think this PR is ready to be merged upstream.

@Rowlando13 Rowlando13 merged commit c9f7d9d into pallets:main Mar 18, 2025
10 of 12 checks passed
@kdeldycke kdeldycke deleted the help-option-fix branch March 19, 2025 12:13
@kdeldycke kdeldycke mentioned this pull request Mar 27, 2025
Merged
6 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2025
@Rowlando13 Rowlando13 added this to the 8.2.0 milestone May 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidism davidism davidism left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
8.2.0
Development

Successfully merging this pull request may close these issues.

Regression: Help option subclassing no longer works with verion 8.1.8
4 participants
@kdeldycke @AndreasBackx @davidism @Rowlando13