Fix reconciliation of envvar with default, flag_value and type parameters for flag options

[kdeldycke]

While digging into internals of flag option management in the context of fixing #2952, I stumbled upon lots of edge-cases.

This is a WIP to identify and fix all these issues, essentially because the absence of value is mixed-up with the value of absence.

This PR:

• Fix reconciliation of default, flag_value and type parameters for flag options.
• Fix parsing of flag's environment variables.
• Tests all combinations of these parameters.

Checklist:

• Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
• Add or update relevant docs, in the docs folder and in code.
• Add an entry in CHANGES.rst summarizing the change and linking to the issue.
• Add .. versionchanged:: entries in any relevant code docs.
• Run pre-commit hooks and fix any issues.
• Run pytest and tox, no tests failed.

Closes:

• Flag value is always true when setting any envvar #2952

Fix edge-cases introduced in:

• Added functionality to set flag_value correctly when using envvar #2788
• flag_value is not taken into account with envvar #2746

Refs:

• Fix flag value set when is_flag=True and type is provided #2930
• Added functionality to set flag_value correctly when using envvar #2788
• flag_value is not taken into account with envvar #2746

[davidism]

I stumbled upon lots of edge-cases.

An extreme understatement! 😆 If we can once and for all consolidate how these arguments all affect each other, that would be amazing.

[davidism]

We should also thoroughly document all the interactions once we settle on something.

[kdeldycke]

I stumbled upon lots of edge-cases.

An extreme understatement! 😆 If we can once and for all consolidate how these arguments all affect each other, that would be amazing.

Ahah! 😄 If I can only increase the coverage and fix a couple of issues, I'll be happy. Refactoring will be easier after some incremental improvements.

This PR is still a WIP. I'll work on it progressively in the next few days and try to push it as far as I can without breaking everything! 😅

[Rowlando13]

@kdeldycke This is excellent! I struggled with just reconciling a few edgecases when I was fleshing out the docs.

[kieranyyu]

Test cases look very comprehensive!

[kdeldycke]

Test cases look very comprehensive!

But I'm not finished yet! Still have some edge-cases to tests! 😅

[kdeldycke]

@kdeldycke This is excellent! I struggled with just reconciling a few edgecases when I was fleshing out the docs.

I'll probably add a couple of lines in the docs once I've ironed out this PR. At least to explain the rules by which to expect the different arguments to behave.

[kdeldycke]

This PR is ready to be reviewed and merged: all tests are passing! :)

[Rowlando13]

@davidism did you review all of this or just leave a comment? If so I am good with merging it. I assume this would go in Click 8.3.0?

[Rowlando13]

@kieranyyu would you mind reviewing this pr?

[kdeldycke]

Given how @davidism is uncomfortable with the removal of default_is_missing, maybe we can target the 8.3 release instead of the 8.2.2 one (also suggested by @Rowlando13)?

[Rowlando13]

@davidism Could you please weigh in? I think 8.3.0 makes sense.

[potiuk]

@davidism Could you please weigh in? I think 8.3.0 makes sense.

This one fixes #2952 - which is very annoying for CI - and reason why we have < 8.2.0 for click in Airflow. So it would be worth to fix it in 8.2.2 and pretty quickly - also beacaue people think this is a FEATURE not a BUG - someone already created a PR documenting behaviour of ENVVAR with flags/bools thinking that it is how it is supposed to be #2988

[potiuk]

BTW. We tested this one in Airflow and it looks good and indeed solves the #2952 issue. I can confirm that.

[davidism]

Oh sorry, I missed the latest messages here. It looks like this doesn't change expected behavior, only fixes issues, so this should be fine for 8.2.2.

[Rowlando13]

@kdeldycke Great work! The community feedback from this already looks very good. You are good to merge into stable. You should have perms, if not then let me know and I will merge!

[kdeldycke]

BTW. We tested this one in Airflow and it looks good and indeed solves the #2952 issue. I can confirm that.

Thanks @potiuk for testing this PR! Airflow being popular means my PR seems sound enough to be merged upstream! :)

@kdeldycke Great work! The community feedback from this already looks very good. You are good to merge into stable. You should have perms, if not then let me know and I will merge!

Ok I'll try to merge it myself. Should be a good test for the process. Thanks for the approval! 👍

[kdeldycke]

Hmmm @Rowlando13 ... What's your preferred way here? A proper merge commit or a rebase? On stable or main? (feel free to do it if you have the answer so we don't waste time with back and forth).

[davidism]

Since it's a bug fix it goes in stable. Since you don't have any work commits and no merge conflicts (you already squashed/rebased), you can do a regular merge commit.

[davidism]

Tagged you in a few other issues that might be affected by this.