> On Sep 16, 2016, at 09:48, Pierre-Yves David <pierre-yves.da...@ens-lyon.org> 
> wrote:
> 
> 
> 
> On 09/14/2016 05:11 AM, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <au...@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.

>> 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.

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

> I see you have a longer reply about handling of the tri-state later in this 
> thread, I'll reply with more details there.
> 
> Cheers,
> 
> -- 
> Pierre-Yves David

_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to