[PATCH 2 of 7 v3 flags] fancyopts: disallow true as a boolean flag default (API)

Pierre-Yves David pierre-yves.david at ens-lyon.org
Fri Sep 16 10:29:17 EDT 2016



On 09/16/2016 04:21 PM, Augie Fackler wrote:
>
>> On Sep 16, 2016, at 09:48, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
>>
>>
>>
>> On 09/14/2016 05:11 AM, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie at google.com>
>>> # Date 1472584421 14400
>>> #      Tue Aug 30 15:13:41 2016 -0400
>>> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>>> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>>> fancyopts: disallow true as a boolean flag default (API)
>>>
>>> This was nonsense, as there's not been a way for the user to
>>> un-specify the flag. Restricting this behavior will open the door to
>>> some nice fit-and-finish functionality in a later patch, and shouldn't
>>> break any third party extensions. This is marked as (API) since it
>>> might break a third party extension, but given the fact that it was
>>> silly before that's mostly a formality.
>>
>> I really wish we had details on this as requested in the review of the previous version. Especially because I remember that forbidding 'True' as default was making other improvement hard so I'm not sure why we have to do this.
>
> Err? I'm not sure what you're asking for now, and I definitely didn't understand it on the last go round.

Okay, let me clarified, My question here is:

Why is this problematic to have default to 'True Value'


You refered this as """These cases are a bit problematic, because we 
don't really have a way to specify default-true flags""" in
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-September/087968.html

I'm seeing another avatar of this question when you says "Restricting 
this behavior will open the door to some nice fit-and-finish 
functionality in a later patch" in the current thread. I would like 
details on this "nice fit-and-finish" features.

Does this clarify my question ?

>>> Due to how early parsing of global options works, we have to add some
>>> extra coupling between fancyopts and dispatch so that we can provide a
>>> "default" of True for already-parsed boolean flags when doing
>>> command-level flag parsing.
>>>
>>> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>>> --- a/mercurial/dispatch.py
>>> +++ b/mercurial/dispatch.py
>>> @@ -555,7 +555,10 @@ def _parse(ui, args):
>>>
>>>     # combine global options into local
>>>     for o in commands.globalopts:
>>> -        c.append((o[0], o[1], options[o[1]], o[3]))
>>> +        # Passing a tuple of length six through into the option parser
>>> +        # allows otherwise-illegal defaults to survive, which is how
>>> +        # we handle global options like --quiet.
>>> +        c.append((o[0], o[1], options[o[1]], o[3], '', True))
>>>
>>>     try:
>>>         args = fancyopts.fancyopts(args, c, cmdoptions, gnu=True)
>>> diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
>>> --- a/mercurial/fancyopts.py
>>> +++ b/mercurial/fancyopts.py
>>> @@ -79,10 +79,22 @@ def fancyopts(args, options, state, gnu=
>>>     alllong = set(o[1] for o in options)
>>>
>>>     for option in options:
>>> -        if len(option) == 5:
>>> +        boolok = False
>>> +        if len(option) == 6:
>>> +            # If the tuple is of length 6, then it's a global option
>>> +            # that was already parsed, so we're really just passing
>>> +            # its "default" through the second phase of flags parsing
>>> +            # (for command-level flags). As a result, we have to allow
>>> +            # defaults of True and not rewrite defaults of False.
>>> +            short, name, default, comment, dummy, dummy = option
>>> +            boolok = True
>>> +        elif len(option) == 5:
>>>             short, name, default, comment, dummy = option
>>>         else:
>>>             short, name, default, comment = option
>>> +        if default is True and not boolok:
>>> +            raise ValueError('fancyopts does not support default-true '
>>> +                             'boolean flags: %r' % name)
>>
>> As pointed in a previous review, identity testing to boolean really raise red flag here. I see this as a significant sign of a bad idea and something we should avoid at all cost.
>
> Calm down - this isn't the one that was pointed out in the past, and I didn't notice this one as I was doing reworks for v3. I don't disagree with your opinion here.

Don't worry, I'm not mad, just a bit puzzled ☺ I'm happy to learn we 
actually agree here.

> It sounds like you should be mailing a check-code rule about 'is (True|False)' to forbid this pattern.

Yep, as we have a consensus to ban them, check-code seems something we 
should do.

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list