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

Regression: Help option subclassing no longer works with verion 8.1.8 #2832

Open
pombredanne opened this issue Dec 30, 2024 · 7 comments · May be fixed by #2840
Open

Regression: Help option subclassing no longer works with verion 8.1.8 #2832

pombredanne opened this issue Dec 30, 2024 · 7 comments · May be fixed by #2840
Labels
bug

Comments

@pombredanne
Copy link
Contributor

Click 8.18 has a regression wrt. to help with Option subclasses.

This was found in this issue originally:

This was introduced in this PR by @kdeldycke 👋 :

It breaks code that uses option subclasses.

  • This code works in 8.1.7 and 8.1.8 with no custom class
import click

@click.command()
@click.help_option("-h", "--help")
def scancode():
    """OK in pkg:pypi/[email protected] and pkg:pypi/[email protected]"""
    pass


if __name__ == "__main__":
    scancode()

and then with 8.1.7 and 8.1.8:

$ python  works.py --help
Usage: works.py [OPTIONS]

  Regression in pkg:pypi/[email protected]

Options:
  -h, --help  Show this message and exit.
  • This code works in 8.1.7 and fails in 8.1.8 with a custom class
import click


class PluggableCommandLineOption(click.Option):
    pass


@click.command()
@click.help_option("-h", "--help", cls=PluggableCommandLineOption)
def scancode():
    """Regression in pkg:pypi/[email protected]"""
    pass


if __name__ == "__main__":
    scancode()

and then with 8.1.7

 python  failing.py --help
Usage: failing.py [OPTIONS]

  Regression in pkg:pypi/[email protected]

Options:
  -h, --help  Show this message and exit.


$ python  failing.py -h
Usage: failing.py [OPTIONS]

  Regression in pkg:pypi/[email protected]

Options:
  -h, --help  Show this message and exit.

and then with 8.1.8

$ python  failing.py -h
Error: Option '-h' requires an argument.


$ python  failing.py --help
Error: Option '--help' requires an argument.
  • This code works more or less in 8.1.7 and 8.1.8 with custom class and no "--help" option
import click


class PluggableCommandLineOption(click.Option):
    pass


@click.command()
@click.help_option("-h", cls=PluggableCommandLineOption)
def scancode():
    """Regression in pkg:pypi/[email protected]"""
    pass


if __name__ == "__main__":
    scancode()

and then with 8.1.7

$ python  works2.py -h
Usage: works2.py [OPTIONS]

  Regression in pkg:pypi/[email protected]

Options:
  -h      Show this message and exit.
  --help  Show this message and exit.

and then with 8.1.7

$ python  works2.py --help
Usage: works2.py [OPTIONS]

  Regression in pkg:pypi/[email protected]

Options:
  -h      Show this message and exit.
  --help  Show this message and exit

and then with 8.1.8

$ python  works2.py -h
Error: Option '-h' requires an argument.

and then with 8.1.8 note the changes in -h TEXT

$ python  works2.py --help
Usage: works2.py [OPTIONS]

  Regression in pkg:pypi/[email protected]

Options:
  -h TEXT
  --help   Show this message and exit.

Environment:

  • Python version: 3.9 to 3.11
  • Click version: 8.1.8
@pombredanne
Copy link
Contributor Author

@kdeldycke I guess that no refactor goes unpunished!

@AyanSinhaMahapatra AyanSinhaMahapatra mentioned this issue Dec 30, 2024
Merged
6 tasks
@Rowlando13 Rowlando13 added the bug label Dec 31, 2024
@pombredanne pombredanne changed the title Regression: Help option sublassing no longer works with verion 8.1.8 Regression: Help option subclassing no longer works with verion 8.1.8 Dec 31, 2024
@pombredanne
Copy link
Contributor Author

The short term fix by @AyanSinhaMahapatra is that we skip using 8.1.8 entirely in https://github.com/aboutcode-org/scancode-toolkit/pull/4043/files#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52R70

click >= 6.7, !=7.0, !=8.1.8

@kdeldycke
Copy link
Contributor

I just started to update Click Extra to the new v8.1.8 (bad release timing 😅), but I've already seen some strange behavior with the help option.

Thanks @pombredanne for the details, I'll investigate the issue and keep you informed.

@kdeldycke
Copy link
Contributor

An immediate solution would be to make the custom class inherit click.decorators.HelpOption instead of click.Option, like so:

class PluggableCommandLineOption(click.decorators.HelpOption):
    pass

kdeldycke added a commit to kdeldycke/click that referenced this issue Jan 9, 2025
@kdeldycke
Allow customization of help option right from its decorator.
569350f 
Removes `click.decorators.HelpOption` class.

Closes pallets#2832.
@kdeldycke kdeldycke linked a pull request Jan 9, 2025 that will close this issue
Open
6 tasks
@kdeldycke
Copy link
Contributor

The root of the problem is that by deduplicating the code generating the help option in 8.1.8, I introduced a HelpOption class. This class carry all the setup necessary to make a standard Option behave like an help option.

The problem is by using a custom class on the @help_option decorator, you bypass all the custom setup made by HelpOption. Hence the discrepency in behavior between 8.1.7 and 8.1.8.

I just made a proposal in PR #2840 to revert the behavior to what it was in 8.1.7.

kdeldycke added a commit to kdeldycke/click that referenced this issue 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 issue 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

@kdeldycke I assume we want to keep the HelpOption in 8.2.0 but just not include it in 8.1.x, right? If so, could you verify this is documented in 8.2.0? I don't think we should stop 8.2.0's release for this, but it's good to have it documented.

@kdeldycke
Copy link
Contributor

In retrospect I think introducing the HelpOption was a mistake.

Yes, it was more in line with the original (duplicated) implementation of the --help option. And it made the code within the get_help_option() less hackish than my proposal in #2840 (see that ugly construct ending with dummy_func.__click_params__.pop()).

But on the other hand we ended up with the @help_option decorator being too dissimilar from the @password_option, @version_option and other pre-configured option proposed by Click.

For the sake of simplicity and keeping the code aligned, I would propose to go with my implementation in #2840 for both 8.1.x and 8.2.0, and forget about changing the option decorators from decorators to classes altogether.

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

Successfully merging a pull request may close this issue.

Move --help option defaults from its class to its decorator kdeldycke/click
4 participants
@kdeldycke @pombredanne @AndreasBackx @Rowlando13