On 09/16/2016 04:21 PM, Augie Fackler wrote:

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.

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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to