[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 09:48:28 EDT 2016



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.

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

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


More information about the Mercurial-devel mailing list