Patchwork [1,of,2,V4] shelve: refactor option combination check to easily add new ones

login
register
mail settings
Submitter Katsunori FUJIWARA
Date June 11, 2014, 1:50 p.m.
Message ID <af2b2360b9dc87167b8f.1402494645@feefifofum>
Download mbox | patch
Permalink /patch/4973/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - June 11, 2014, 1:50 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1402494363 -32400
#      Wed Jun 11 22:46:03 2014 +0900
# Node ID af2b2360b9dc87167b8ffe729342794b8acd115a
# Parent  0f73ed6293629f69aa2f01d8940e91faeded49ae
shelve: refactor option combination check to easily add new ones

Before this patch, the name of a newly added option had to be added
into each string that was passed to the 'checkopt()' internal
function: these are white-space-separated list of un-acceptable option
names.

This new option had to be added into multiple strings because each
option could belong to only one category of 'create', 'cleanup',
'delete' or 'list'.

In addition to this redundancy, each string passed to 'checkopt()' was
already too long to include a new one.

This patch refactors option combination check to make it easier to add
a new option in a subsequent patch.

Now, 'checkopt()' only takes one category ('cleanup', 'delete' or
'list'), and checks whether all explicitly activated options are
allowed by it or not (if specified category is activated in 'opts').

The 'date' entry is listed in 'allowableopts', but commented out,
because:

  - 'date' shouldn't be checked for test

    checking 'date' causes unexpected failure of 'test-shelve.t',
    because 'run-test.py' puts "[default] shelve = --date '0 0'" into
    hgrc.

  - explicitly listing it can advertise that ignoring it is intentional
Katsunori FUJIWARA - June 12, 2014, 4:18 p.m.
At Wed, 11 Jun 2014 22:20:27 -0500,
Kevin Bullock wrote:
> 
> On Jun 11, 2014, at 8:50 AM, FUJIWARA Katsunori <foozy@lares.dti.ne.jp> wrote:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1402494363 -32400
> > #      Wed Jun 11 22:46:03 2014 +0900
> > # Node ID af2b2360b9dc87167b8ffe729342794b8acd115a
> > # Parent  0f73ed6293629f69aa2f01d8940e91faeded49ae
> > shelve: refactor option combination check to easily add new ones
> > 
> > Before this patch, the name of a newly added option had to be added
> > into each string that was passed to the 'checkopt()' internal
> > function: these are white-space-separated list of un-acceptable option
> > names.
> > 
> > This new option had to be added into multiple strings because each
> > option could belong to only one category of 'create', 'cleanup',
> > 'delete' or 'list'.
> > 
> > In addition to this redundancy, each string passed to 'checkopt()' was
> > already too long to include a new one.
> 
> The old way, though cumbersome, at least reads in a straightforward
> way to me. I can't verify by staring at the code that your new way
> actually does the same thing.
> 
> What about constructing the whitelist as:
> 
>     {
>         'create': ['addremove', 'date', 'message', 'name']
>         # ...
>     }
> 
> and then looping over the array of compatible(?) options for the
> option we're checking, looking for each one in `opts`?

Would you suppose code like below ?

    ====================
    whitelists = {
        'cleanup': ['cleanup'],
        'delete': ['delete'],
        'list': ['list', 'patch', 'stat'],
    }
    def checkopt(opt):
        if opts[opt]:
            whitelist = whitelists[opt]
            for i in opts:
                if opts[i] and i not in whitelist:
                    raise util.Abort(_("options '--%s' and '--%s' may not be "
                                       "used together") % (opt, i))
            return True
    ====================

To avoid raising exception for global options, this implementation has
to:

  - make each white lists include also global options, or
  - examine "i not in white list and i_is_not_global_opts"


IMHO, "black list" seems to be better than "white list", at least in
this case.


BTW, 'allowableopts' should be a list of tuple '(option, allowed)',
because it is not used for looking something up.

I'll fix this in next (V5) post.

> > +    allowableopts = { # 'create' is pseudo option
> > +        'addremove': 'create',
> > +        'cleanup': 'cleanup',
> > +#        'date': 'create', # ignored for passing '--date "0 0"' always in tests
> > +        'delete': 'delete',
> > +        'list': 'list',
> > +        'message': 'create',
> > +        'name': 'create',
> > +        'patch': 'list',
> > +        'stat': 'list',
> > +    }


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -675,20 +675,31 @@ 
     '''
     cmdutil.checkunfinished(repo)
 
-    def checkopt(opt, incompatible):
+    allowableopts = { # 'create' is pseudo option
+        'addremove': 'create',
+        'cleanup': 'cleanup',
+#        'date': 'create', # ignored for passing '--date "0 0"' always in tests
+        'delete': 'delete',
+        'list': 'list',
+        'message': 'create',
+        'name': 'create',
+        'patch': 'list',
+        'stat': 'list',
+    }
+    def checkopt(opt):
         if opts[opt]:
-            for i in incompatible.split():
-                if opts[i]:
+            for i, a in allowableopts.iteritems():
+                if opts[i] and a != opt:
                     raise util.Abort(_("options '--%s' and '--%s' may not be "
                                        "used together") % (opt, i))
             return True
-    if checkopt('cleanup', 'addremove delete list message name patch stat'):
+    if checkopt('cleanup'):
         if pats:
             raise util.Abort(_("cannot specify names when using '--cleanup'"))
         return cleanupcmd(ui, repo)
-    elif checkopt('delete', 'addremove cleanup list message name patch stat'):
+    elif checkopt('delete'):
         return deletecmd(ui, repo, pats)
-    elif checkopt('list', 'addremove cleanup delete message name'):
+    elif checkopt('list'):
         return listcmd(ui, repo, pats, opts)
     else:
         for i in ('patch', 'stat'):