New "default" behaviour in Click 8.3.x is broken for negative boolean flags
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-xyzthat default to being "off"? This solution should not require flipping the logic of the application, i.e. using adisable_xyzvariable name instead (which indeed simplifies the Click option definition but creates double-negatives in the user code that wants to checkif 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
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.
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
With regard to what you said at the top, you are correct the flag/no-flag option is the way to go.