Ensure that type_cast_value never gets UNSET
#3090
Conversation
In order to ensure that user-defined `type_cast_value` hooks are never called with a value of `UNSET`, it is necessary to intercept `UNSET` values and, in the particular cases in which `UNSET` needs special handling, preempt any call to `type_cast_value()`. `process_value()` is updated to check before calling `type_cast_value` and to skip the call entirely if an `UNSET` value is detected. Passing a value of `None` would not have the same effect for `click.Option`, which previously had logic which is specific to the `value is UNSET` case, and which cannot be reliably detected if a `None` value is passed in. This means that `type_cast_value()` is no longer guaranteed to be called during argument processing, but when it is called, it's guaranteed not to see an `UNSET` value. New tests explore this only from the perspective of checking if `UNSET` is ever passed, not in terms of pinning down when/if the hook gets called. resolves pallets#3069
kdeldycke
left a comment
kdeldycke
left a comment
There was a problem hiding this comment.
OK I understand the trade off and I am ok with them given that this PR:
- pragmaticcaly solve #3069
- adds dozen of new unit tests over uncovered edge-cases
It is not worse than the current state, as:
- in-place
multiplechecks and normalization for tuples already happens a lot (i.e.self.multiple or self.nargs == -1) - other in-place checks on
UNSEThave been added in upcoming 8.3.1 for compatibility reasons
Overall it is a net gain as it will keep the community happy and provide grounds for upcoming refactors in 9.x.
| :attr:`type`, :attr:`multiple`, and :attr:`nargs`. | ||
| """ | ||
| if value in (None, UNSET): | ||
| if value is None: |
There was a problem hiding this comment.
I like this as it returns type_cast_value to its original code before my #3030 PR.
kdeldycke
changed the title
type_cast_value never gets UNSET
|
Oh and also: I checked this branch against some private code and I could not find any issues. |
|
Looking in my uncommitted tentative code of refactoring some part of the option parsing, I also identified the |
|
This PR is ready to be merged upstream. |
|
It would be nice if you released version 8.3.1 with this fix. |
4 participants
In order to ensure that user-defined
type_cast_valuehooks are nevercalled with a value of
UNSET, it is necessary to interceptUNSETvalues and, in the particular cases in which
UNSETneeds specialhandling, preempt any call to
type_cast_value().process_value()is updated to check before callingtype_cast_valueand to skip the call entirely if an
UNSETvalue is detected. Passinga value of
Nonewould not have the same effect forclick.Option,which previously had logic which is specific to the
value is UNSETcase, and which cannot be reliably detected if a
Nonevalue ispassed in.
This means that
type_cast_value()is no longer guaranteed to be calledduring argument processing, but when it is called, it's guaranteed not to
see an
UNSETvalue. New tests explore this only from the perspective ofchecking if
UNSETis ever passed, not in terms of pinning down when/ifthe hook gets called.
resolves #3069
I think this change is worth considering as a fix for #3069, but I'm not
certain that it's good because it seems to trade one kind of incompatibility,
whether or not
UNSETgets passed, for a different one: whether or nottype_cast_value()gets called at all.If
type_cast_value()is supposed to never see anUNSETvalue, andprocess_valueis "holding anUNSETvalue, then you might think that allwe need is
but
Option.type_cast_valuethrows a wrench in that, since it needs tosee the
UNSETsentinel to properly determine its behavior.I chose to take the same approach for handling vis-a-vis
multipleandnargs=-1inParameter, on the grounds that it seemed "better aligned" withwhat has to happen in
Option.