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

[ldionne]

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]

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.

[Rowlando13]

There are some complex behaviors around using flag_value and default. Not all the edge cases are worked out. And some 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]

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

[ThiefMaster]

I think we should at least document this breakage, since it's likely that people do not have such things covered with tests, and depending on what the flag does it can be dangerous if the default seen by the application suddenly changes.

FWIW, I also think we should fix this, and come up with a way to have a "default on, provide option to turn off" type flag. I don't think providing both an on and and off version (--foo/--no-foo) is the best way to go, since it doesn't necessarily make sense to have --foo which would always be a no-op when the default is True anyway.

[Rowlando13]

Sure. I am up for whatever docs you have in mind.

[Rowlando13]

I think that generally make sense, though I don't know of a specific way to fix it. The currently flag situation is a bit gnarly.

[kdeldycke]

Can confirm the situation.

There is a core design tension because default has currently two meanings for flags:

1. "Pass this Python value to my function if the flag isn't used", i.e. when default=True then function gets True
2. "Activate this flag by default", i.e. when default=True then function gets flag_value

In this issue @ldionne needs the first behavior. But currently we do 2). We arbitraged for the second behavior to not break the --reload/--no-reload pattern in Flask (see: #3024).

I resolved the None vs "not set" ambiguity with my UNSET, but True vs "activate by default" is another beast. I don't think there is a way to reconcile the two behaviors. This is a design issue, that we can maybe solve in a breaking 9.x series.

In the mean time, @ldionne ideas of raising an explicit error looks not bad to me: I'd rather have something that reduce the surprise from the user. With an explicit message, and a good explanation in the docs to points people to. This will reduce the support burden while we think deeply about 9.x.

[kdeldycke]

For context, the oldest issue I can find dates back from 2022, and as pointed by a user, to be in click since forever: #2351

And we are aware of this issue, as we reduced in the documentation the strong language around this behavior in this PR: #3049

So in the absence of a technical solution, we need to choose how to communicate to the users and developers the situation.