Fix default handling to defer UNSET normalization

[sirosen]

When options are being parsed, multiple options may evaluate to have defaults of UNSET, and some may evaluate to have explicitly set defaults of None. The normalization of UNSET -> None is now deferred until all options have been parsed, so that it is possible to accurately evaluate and save the first non-UNSET default value.

A new test exercises this functionality by pointing two flags at the same parameter declaration, and showing that their behavior is not order sensitive when only one of the flags has a default.

resolves #3071

---

The PR template says to add a note in CHANGES.rst for the change, but there's no header for the next release, so I wasn't sure where to put a change note. If a maintainer gives me a pointer in terms of what to do, I'm happy to amend with a change note. 🙂

[kdeldycke]

Your unit tests clearly demonstrate the issue and your changes don't trigger any other tests covering the default behavior. So for me this is enough to prove that your PR is really fixing the issue.

I'm pretty sure there is a more elegant way to solve it but for inclusion in a 8.3.1 hotfix release, this is good to me. We'll have plenty of time to refactor the code later. At this stage, I consider Click is still in the accumulation phase of external use-cases. So more unit tests the better! :)

[sirosen]

I'm pretty sure there is a more elegant way to solve it

I agree, but the way I see it that's a change for 8.4 or 9.0 .

There are too many different places which are independently responsible for choosing between UNSET and None. I think the right solve is to separate the data which is stored (where you might have UNSET) from the data which is viewed.

For example, changes like this:

class Option(Parameter):
    _default: t.Any | UNSET

    @property
    def default(self):
        if self._default is UNSET:
            return None
        return self._default

But I don't know how that looks across all usage sites.

[Rowlando13]

@sirosen I added the new section for you. In the future, you can just add a new section. If it is not there, it just means the maintainer who released forgot to add it after releasing.

[kdeldycke]

There are too many different places which are independently responsible for choosing between UNSET and None. I think the right solve is to separate the data which is stored (where you might have UNSET) from the data which is viewed.

100% this. I could not find an way to do this cleanly in #3030 so I had to draw the line. Addressing this is indeed a refactor.

In the mean time, your fix is in line with the policy we adopted:

• to hide UNSET in to_info_dict() method in the original PR:
  • https://github.com/pallets/click/pull/3030/files#diff-11ba83cac151f7b24a1ed7c31a2a522d24d190cfa43199aa478d1e9cd2e6c610L2677-R2820
  • Differentiate absence of value and value of absence for default and flag_value arguments #3030 (comment)
• to hide UNSET in the callback in the original PR:
  • 029bbed
  • Differentiate absence of value and value of absence for default and flag_value arguments #3030 (comment)

So once you tweaked the code comments in the review I left you, this PR is to me good to be merged in a 8.3.1 hotfix release.

[sirosen]

I rebased and added the missing changelog note. I think this is ready!

[kdeldycke]

Good call @Rowlando13 on merging this! :)