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

[FUJIWARA Katsunori]

At Wed, 11 Jun 2014 22:20:27 -0500,
Kevin Bullock wrote:
> 
> On Jun 11, 2014, at 8:50 AM, FUJIWARA Katsunori <foozy at lares.dti.ne.jp> wrote:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy at 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 at lares.dti.ne.jp