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

[Pierre-Yves David]

On 09/14/2016 05:52 PM, Augie Fackler wrote:
> (sorry for the length of this reply)
>
>> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya at tcha.org> wrote:
>>
>> On Wed, 14 Sep 2016 11:05:40 -0400, Augie Fackler wrote:
>>>> On Sep 14, 2016, at 11:03, Yuya Nishihara <yuya at tcha.org> wrote:
>>>>
>>>> On Tue, 13 Sep 2016 23:11:18 -0400, 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)
>
> [...]
>
>> But that can be changed incrementally. We need tristate for diffopts because
>> they are computed from command and config options, but many other commands take
>> None and False as the same value so they can remain to default to None/False.
>>
>> I think --no-git is the most important stuff, which won't be difficult to
>> port to <unset> since opts are processed by patch.diff*opts().
>
> Is the idea something like:
>
> fancyopts.py:
>
> unsetbool = object()
>
> in commands.py:
>
>  diffopts = [
> -     ('a', 'text', None, _('treat all files as text')),
> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
> -     ('g', 'git', None, _('use git extended diff format')),
> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
> -     ('', 'nodates', None, _('omit dates from diff headers'))
> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
>  ]
>
> And then we could synthesize negated versions only for booleans that have fancyopts.unsetbool as the default? That'd be less generic (and so require a lot more cleanup work on a per-option basis), but maybe that's a good thing? Note that changing these entries in diffopts do stand to be at least a little bit risky for extensions (they may not be ready for the fancyopts.unsetbool part of the tai-state), but I don't think that's something we should worry about overmuch.
>
> I could also see room for something like
>
> class unsetbool(object):
>   default = True
>   def __init__(self, v):
>     self._v = v
>   def __bool__(self):
>     return self._v
>
> and then you'd have your default value be specified as fancyopts.unsetbool(True|False). I don't like this as well, but I can't put my finger on why (a previous round of this series between v2 and v3 actually went down this route and it got somewhat messy, but I was trying to be more generic and automatic about it).
>
> Regardless, both of these new proposed approaches still leave us in the awkward place of having to figure out how to describe the default for a boolean flag in the help.
>
>
> If we go ahead with what I've got, that pretty well ties our hands in the future. I'd still rather do this[1] (it's already done, and means boolean flags are globally consistent behavior-wise) than have to go through and tweak the default value on nearly boolean flag in the codebase (and still have extensions get zero benefit). It also makes documentation simpler[0] - the default mode of a flag is the opposite of what shows up in help (so if the default of 'check' is false, then we show --check, but if the default is --check, we describe the flag as --no-check in the help output.)
>
> How do people want to proceed?

I'm really opposed to having identity testing for True and False. This 
would not prevent us to use 'None' as 'default value' (I'm fine with 
identity testing with None) but that prevent 'default to True' setting 
which seems unfortunate. This is also less flexible for future evolution.

I think having a dedicated 'booleanobject' for the default value make 
senses here. It keeps all the existing code unchanged but for the place 
that are explicitly interested in checking for command line override. 
This is probably my preferred solution.

The could have a 'defaulttrue' and 'defaultfalse' value, with some 
'defaultvalue' attribute set to True for the code who care.

We could either update the existing definition to use these, of simply 
doing what you have done and have fancyopt to replace the value on the 
Fly when it see them (probably my preferred solution)

Supporting default to True seem useful even if mostly rare. Regarding 
documentation, when default is True, I think we could either:
- display: the --no-foobar version in the help
- add '(set by default)' to te help.

Using object for default value give use for flexibility. We could even 
imagine a future improvement were options have their config counterpart 
directly handled during option parsing, returning a slightly different 
object to keep track of the origin of that 'default' value. This would 
allow the help to report config based override to the user.

Cheers,

-- 
Pierre-Yves David