Flag value is always true when setting any envvar

[alfechner]

Click 8.2.1 includes fixes related to boolean flags.

We’re encountering a specific issue described in this comment. The comment appears in the thread for a different but related issue (#2897), focused on a separate aspect of boolean flag handling. Maybe the issue was overlooked because the discussion centered on the main issue.

[davidism]

When opening an issue, please include all details, including a descriptive title and a minimal verifiable example, in the issue itself.

[alfechner]

I'm using the envvar option and I set the envvar to False, however it's always true for me.

# hi.py
import click

@click.command()
@click.option("--dry-run/--no-dry-run", default=False, envvar="DRY_RUN")
def main(dry_run: bool):
    print(type(dry_run))
    print(f"Dry run: {dry_run}")

if __name__ == "__main__":
    main()

export DRY_RUN=false

python hi.py

Output

<class 'bool'>
Dry run: True

[davidism]

@kieranyyu you've recently become the flag value expert, any chance you could take a look at this one? No worries if not!

It looks like flag_value is True and that's triggering incorrect behavior in resolve_envvar_value during handle_parse_result. Not sure what it was before the recent change, or what it's "supposed" to be.

[kieranyyu]

This behavior is expected given #2788. According to #2746, the flag value should not be read from the environment variable value. Instead, the flag takes on the default value if it is not passed, and takes on the negation of the default value if it is passed. In your example, since you have set the DRY_RUN env variable, this is considered as passing the flag and thus setting it to True. In my opinion this behavior makes sense and is typical for flag parameters.

[alfechner]

@kieranyyu I agree with your general statement about flag values.

However, click supports --dry-run/--no-dry-run. While there is a an option to explicitly disable the dry run with --no-dry-run there is no way to do that (anymore) with the envvar="DRY_RUN".

The behaviour is inconsistent and a breaking change.

I actually think that there is no value in --no-dry-run, nobody would ever use it. Only --dry-run. So relying on the presence of the flag seems to be fine for me. As for the envvar, it’s much more intuitive to use true/false as values rather than relying on the presence of the variable to indicate true and its absence to indicate false, which is less explicit and can easily lead to confusion or mistakes.

If this isn’t going to be reverted to the previous behavior, it would be helpful to have it clarified and officially documented, so we can confidently adjust our code without worrying about future changes.

Thanks for looking into this and all your work on this amazing library!

[kdeldycke]

I can confirm the issue as a regression introduced by Click 8.2.1.

I developed an extensive suite of unittests in Click Extra to cover edge cases of envvar handling in Click. Essentially around case-sensitivity and boolean values.

I can try to port them for Click 8.2.2.

[kdeldycke]

Also relates to: https://github.com/pallets/click/issues/2483

[kieranyyu]

@alfechner I see your point about what is expected here. I can take a look into how we can satisfy both this issue and the other that introduced the regression.

@kdeldycke Thanks for the additional tests, they were helpful to look over to understand the issue.

[kdeldycke]

I just realized there a lot of uncovered edge-cases with flag options, so I'm attempting to fix all of these in https://github.com/pallets/click/pull/2956

[alfechner]

@kieranyyu thanks! I don't know how "far" you want to go but how about considering to drop support for this kind of options: --dry-run/--no-dry-run? I really have a hard time to see the value. As I said before, the negative case will never be set IMHO.

[davidism]

We're not dropping support for positive/negative flags. There are many use cases including:

• Defaults can be dynamic.
• Shell scripts sometimes want to be explicit even when it's the default.
• Shell aliases can set a flag, then an invocation can add a negation of the flag.

[ecs-jnguyen]

Just adding my 2 cents. I noticed the issue was introduced in click 8.2.0. That's when I made the comment in the other issue.

# hi.py
import click

@click.command()
@click.option("--dry-run/--no-dry-run", default=False, envvar="DRY_RUN")
def main(dry_run: bool):
    print(type(dry_run))
    print(f"Dry run: {dry_run}")

if __name__ == "__main__":
    main()

➜  pip list
Package    Version
---------- -------
click      8.2.0
pip        24.0
setuptools 65.5.0

➜  export DRY_RUN="False"

➜  python hi.py
<class 'bool'>
Dry run: True

[Rowlando13]

We're not dropping support for positive/negative flags. There are many use cases including:

* Defaults can be dynamic.

* Shell scripts sometimes want to be explicit even when it's the default.

* Shell aliases can set a flag, then an invocation can add a negation of the flag.

This is great info for the docs. I really could not think of use cases for the flag/negation flag pattern.

[sboardwell]

I have found a work around based on the closed https://github.com/pallets/click/pull/2812 addressing https://github.com/pallets/click/issues/750

TLDR;

• putting the boolean default value in quotes fixes the problem.
• it also means the envvar vaue is validated as a boolean

I'm not sure what is supposed to be fixed and how, but the info is hopefully useful.

EDIT: adding class in output - bool type is found automatically, no need to add , type=click.BOOL

Testing:

#!/usr/bin/env python3
import click
import sys

def main():
    """Main entry point for the CLI."""
    @click.group()
    def cli():
        """A simple CLI to demonstrate environment variable handling."""
        pass

    cli.add_command(infostr)
    cli.add_command(infobool)

    cli()

@click.command()
@click.option('-s', '--shout/--no-shout', default="False", help="Shout the platform name", envvar="SHOUT")
def infostr(shout):
    rv = sys.platform
    if shout:
        rv = rv.upper() + '!!!!'
    click.echo(f"{rv} and arg class is {type(shout)}")

@click.command()
@click.option('-s', '--shout/--no-shout', default=False, help="Shout the platform name", envvar="SHOUT")
def infobool(shout):
    rv = sys.platform
    if shout:
        rv = rv.upper() + '!!!!'
    click.echo(f"{rv} and arg class is {type(shout)}")

if __name__ == "__main__":
    main()

Running the command:

{
  for cmd in "infobool" "infostr"; do
    echo "============================"
    echo "Testing command: $cmd"
    echo "============================"
    for arg in "invalid" "true" "1" "false" "0"; do
      echo "SHOUT=$arg python shout.py $cmd"
      echo "$(SHOUT=$arg python shout.py $cmd 2>&1)"
      echo "----------------------------"
    done
    echo "python shout.py $cmd --shout"
    echo "$(python shout.py $cmd --shout 2>&1)"
    echo "----------------------------"
    echo "python shout.py $cmd --no-shout"
    echo "$(python shout.py $cmd --no-shout 2>&1)"
    echo "----------------------------"
    echo "python shout.py $cmd -s"
    echo "$(python shout.py $cmd -s 2>&1)"
  done
}

Output:

• infobool has a number of incorrect values
• infostr works as expected

============================
Testing command: infobool
============================
SHOUT=invalid python shout.py infobool
LINUX!!!! and arg class is <class 'bool'>
----------------------------
SHOUT=true python shout.py infobool
LINUX!!!! and arg class is <class 'bool'>
----------------------------
SHOUT=1 python shout.py infobool
LINUX!!!! and arg class is <class 'bool'>
----------------------------
SHOUT=false python shout.py infobool
LINUX!!!! and arg class is <class 'bool'>
----------------------------
SHOUT=0 python shout.py infobool
LINUX!!!! and arg class is <class 'bool'>
----------------------------
python shout.py infobool --shout
LINUX!!!! and arg class is <class 'bool'>
----------------------------
python shout.py infobool --no-shout
linux and arg class is <class 'bool'>
----------------------------
python shout.py infobool -s
LINUX!!!! and arg class is <class 'bool'>
============================
Testing command: infostr
============================
SHOUT=invalid python shout.py infostr
Usage: shout.py infostr [OPTIONS]
Try 'shout.py infostr --help' for help.

Error: Invalid value for '-s' / '--shout': 'invalid' is not a valid boolean.
----------------------------
SHOUT=true python shout.py infostr
LINUX!!!! and arg class is <class 'bool'>
----------------------------
SHOUT=1 python shout.py infostr
LINUX!!!! and arg class is <class 'bool'>
----------------------------
SHOUT=false python shout.py infostr
linux and arg class is <class 'bool'>
----------------------------
SHOUT=0 python shout.py infostr
linux and arg class is <class 'bool'>
----------------------------
python shout.py infostr --shout
LINUX!!!! and arg class is <class 'bool'>
----------------------------
python shout.py infostr --no-shout
linux and arg class is <class 'bool'>
----------------------------
python shout.py infostr -s
LINUX!!!! and arg class is <class 'bool'>

[peanutsven]

@click.option("-d", "--dryrun", is_flag=True, default="False", envvar="DRYRUN")

If i use the is_flag argument the option foo.py --dryrun retuns False.

[potiuk]

We just hit this issue today with Airflow after dependabot updated our limits. We are going to make a PR shortly to bring back the old behaviour for bool flag (generally eval envvaar for flags and bools and treat "falsey" values as false I understand @davidism that this is what you want to bring back this behaviour?

[davidism]

Yes, this will be fixed in the next fix release.

[potiuk]

Yes, this will be fixed in the next fix release.

Do you need a PR :)?

[davidism]

I think #2956 is handling it? You could review that/try it out.

[potiuk]

Ah cool. Let me test it with Airflow settings :)

[potiuk]

Yep. Confirmed #2956 works locally and I run the complete CI suite here https://github.com/apache/airflow/pull/52416

THis is where it failed, as we are deriving the flags in complex-ish ways looking at the files coming in PR) - that's why we use env-vars to pass values.

[gopidesupavan]

Cool thanks @davidism

[kdeldycke]

A fix has been merged into the stable branch and will be available in the upcoming 8.2.2 release! 🎉