click icon indicating copy to clipboard operation
click copied to clipboard

New "default" behaviour in Click 8.3.x is broken for negative boolean flags

Open ldionne opened this issue 2 months ago • 3 comments

The behaviour of default=... for flag values changed in Click 8.3.0, as documented in the release notes. The new behaviour is broken for negative flags, unless I am holding it wrong. Here's a minimal reproducer (put this in foo.py):

import click

@click.command('foo')
@click.option('--without-xyz', 'enable_xyz',
              help="Disable xyz", flag_value=False, default=True, show_default=True)
def foo(enable_xyz):
    print(f'enable_xyz = {enable_xyz}')

foo()

The intent here is that we have an argument representing whether xyz is enabled, and a flag named --without-xyz which defaults to being disabled. Here, "disabled" means that the flag is not passed, which means the default=True value is used to initialize the enable_xyz variable. If --without-xyz were passed, the flag_value=False would be used to initialize enable_xyz instead, which makes sense. Or at least, that was the case in Click 8.2.x:

% ./foo.py
enable_xyz = True

 % ./foo.py --without-xyz
enable_xyz = False

However, with Click 8.3.0, default=True is now being treated as a special case. Special cases are often surprising, and indeed here this leads to the following behaviour:

% ./foo.py
enable_xyz = False

% ./foo.py --without-xyz
enable_xyz = False

I don't see how the new behaviour makes sense, at least for negative flags. I do not understand why a special case is needed for flags, as documented:

But there is a special case for flags. If a flag has a flag_value, then setting default=True is interpreted as the flag should be activated by default. So instead of the underlying function receiving the True Python value, it will receive the flag_value.

Is there a reason for this special case?

FWIW, this is a pretty major breaking change that results in the negative flag being always unset. This will often result in a silent change in behaviour for software that used to work as intended. Such software may keep functioning (depending on what --without-xyz was disabling), but may change behaviour, performance characteristics, etc. This is pretty bad -- I would have much much preferred a loud error to this silent behaviour change.

Anyway, here are some thoughts:

  • Is the special case really useful and necessary?
  • Would it make sense to make this an explicit error instead of silently changing behaviour in Click 8.3.x?
  • If the Click developers find that this is working as designed, what's the canonical way of adding options like --without-xyz that default to being "off"? This solution should not require flipping the logic of the application, i.e. using a disable_xyz variable name instead (which indeed simplifies the Click option definition but creates double-negatives in the user code that wants to check if not disable_xyz).
  • Once the path forward is decided, I believe that a documentation update might be helpful to at least cover how negative options should be done.

Cheers and sorry in advance in case I missed something in the documentation that explains my confusion.

Environment:

  • Python version: Python 3.10 (but it should reproduce anywhere)
  • Click version: 8.3.x

ldionne avatar Oct 16 '25 18:10 ldionne

While looking into this some more, I think I answered part of my question. While reading https://click.palletsprojects.com/en/stable/options/#boolean, I'm not certain why but I got the impression that --xyz/--no-xyz would only work for options that followed exactly that pattern (with a --no- prefix). Playing around with it and re-reading the documentation, I see that something like --with-xyz/--without-xyz also works, which (IMO) answers the question of the best practice to handle this. In my case, I should be using:

@click.command('foo')
@click.option('--with-xyz/--without-xyz', 'enable_xyz',
              help="Whether to enable xyz", default=True, show_default=True)
def foo(enable_xyz):
    print(f'enable_xyz = {enable_xyz}')

However, my comments about this being a silent breaking change and the new behaviour not being useful still hold. IMO, if we all agree that --xyz/--no-xyz is the way to go, I would suggest making it an explicit error to use a boolean flag_value in combination with a boolean default for a flag. I'm not sure there is a non-confusing use case for doing that:

@click.option('--xyz', flag_value=True, default=True)
# ❌ This is nonsensical, True is always passed to the function.

@click.option('--xyz', flag_value=True, default=False)
# ❌ This is the default if you don't pass any `flag_value` or `default`.
# Not being able to set it explicitly is probably acceptable?

@click.option('--xyz', flag_value=False, default=True)
# ❌ This is the case described in this issue.
# With Click 8.3, this ends up always passing False to the function.

@click.option('--xyz', flag_value=False, default=False)
# ❌ This is nonsensical, False is always passed to the function.

So I don't think there is any useful case for using flag_value and default with boolean values in the current state of things.

ldionne avatar Oct 16 '25 19:10 ldionne

There are some complex behaviors around using flag_value and default. Not all the edge cases are worked out. And some don't fail silently when they should not. I am not sure of a perfect resolution since the behavior has been out there for a while. Currently the general idea is that flag_value passes value to the underlying function and default is used to indicate what the default flag is. This is a special case use of default, since usually default is just passed in if nothing is given

@click.option('--xyz', flag_value=True, default=True)

Seems straightforward but what about this:

@click.option('--xyz', flag_value=True, default=True)
@click.option('--xyz-off', flag_value=False)

This currently works and is sensible. The same is true for option 3 that you presented. I am also not sure from a parsing perspective how exactly to handle the extra complexity of setting default=False. Should it generate an error under when you then pass in this:

@click.option('--xyz', flag_value=True, default=True)
@click.option('--xyz-off', flag_value=False)
@click.option('--xyz-none', flag_value=None, default=False)

Since you passed in default=False to one but not the other. I think mostly likely it should be a parsing error since you did pass in default=False and that behavior is not really defined. The relevant docs

Rowlando13 avatar Oct 20 '25 06:10 Rowlando13

With regard to what you said at the top, you are correct the flag/no-flag option is the way to go.

Rowlando13 avatar Oct 20 '25 06:10 Rowlando13