Patchwork [2,of,3,V2] shelve: refactor option combination check to add new one easily

login
register
mail settings
Submitter Katsunori FUJIWARA
Date June 5, 2014, 2:12 p.m.
Message ID <c90c042600bcb2810836.1401977524@feefifofum>
Download mbox | patch
Permalink /patch/4938/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - June 5, 2014, 2:12 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1401977395 -32400
#      Thu Jun 05 23:09:55 2014 +0900
# Node ID c90c042600bcb28108366b8c08a06d9ac3259ab7
# Parent  804a341610d154c8a2a4d3d63e36ff2cf08acc4f
shelve: refactor option combination check to add new one easily

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

The name of new option should be added into multiple strings, because
every options can belong to only one category of 'create', 'cleanup',
'delete' or 'list'.

In addition of this redundancy, each strings passed to 'checkopt()'
are already too long to include new one.

This patch refactors option combination check to add new one easily in
succeeding patch.

New 'checkopt()' takes only one of categories ('cleanup', 'delete' or
'list'), and checks whether option allowed only for other categories
is specified or not, if specified category is activated in 'opts'.

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

  - but explicit listing it up can advertise that this ignoring is
    intentional
Pierre-Yves David - June 5, 2014, 11:27 p.m.
On 06/05/2014 07:12 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1401977395 -32400
> #      Thu Jun 05 23:09:55 2014 +0900
> # Node ID c90c042600bcb28108366b8c08a06d9ac3259ab7
> # Parent  804a341610d154c8a2a4d3d63e36ff2cf08acc4f
> shelve: refactor option combination check to add new one easily
>
> Before this patch, the name of the newly added option should be added
> into each strings to be passed to 'checkopt()' internal function:
> these are white-space-separated list of un-acceptable option names.
>
> The name of new option should be added into multiple strings, because
> every options can belong to only one category of 'create', 'cleanup',
> 'delete' or 'list'.
>
> In addition of this redundancy, each strings passed to 'checkopt()'
> are already too long to include new one.
>
> This patch refactors option combination check to add new one easily in
> succeeding patch.
>
> New 'checkopt()' takes only one of categories ('cleanup', 'delete' or
> 'list'), and checks whether option allowed only for other categories
> is specified or not, if specified category is activated in 'opts'.
>
> '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.
>
>    - but explicit listing it up can advertise that this ignoring is
>      intentional

I'm unsure about the english in this description. I would like a native 
speaker to decide between "it's ok lets queue this" and "This needs a 
resend"
Sean Farley - June 5, 2014, 11:59 p.m.
FUJIWARA Katsunori writes:

> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1401977395 -32400
> #      Thu Jun 05 23:09:55 2014 +0900
> # Node ID c90c042600bcb28108366b8c08a06d9ac3259ab7
> # Parent  804a341610d154c8a2a4d3d63e36ff2cf08acc4f
> shelve: refactor option combination check to add new one easily
>
> Before this patch, the name of the newly added option should be added
> into each strings to be passed to 'checkopt()' internal function:
> these are white-space-separated list of un-acceptable option names.
>
> The name of new option should be added into multiple strings, because
> every options can belong to only one category of 'create', 'cleanup',
> 'delete' or 'list'.
>
> In addition of this redundancy, each strings passed to 'checkopt()'
> are already too long to include new one.
>
> This patch refactors option combination check to add new one easily in
> succeeding patch.
>
> New 'checkopt()' takes only one of categories ('cleanup', 'delete' or
> 'list'), and checks whether option allowed only for other categories
> is specified or not, if specified category is activated in 'opts'.
>
> '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.
>
>   - but explicit listing it up can advertise that this ignoring is
>     intentional

Yes, this seems fine to me. If you want something to bikeshed (Katsunori
should verify this):

========
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 new one.

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

Now, 'checkopt()' only takes one category ('cleanup', 'delete' or
'list'), and checks whether the option allowed for other categories is
specified 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.

  - explicit listing it can advertise that ignoring it is intentional
Katsunori FUJIWARA - June 6, 2014, 1:24 p.m.
At Thu, 05 Jun 2014 18:59:06 -0500,
Sean Farley wrote:
> 
> 
> FUJIWARA Katsunori writes:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1401977395 -32400
> > #      Thu Jun 05 23:09:55 2014 +0900
> > # Node ID c90c042600bcb28108366b8c08a06d9ac3259ab7
> > # Parent  804a341610d154c8a2a4d3d63e36ff2cf08acc4f
> > shelve: refactor option combination check to add new one easily
> >
> > Before this patch, the name of the newly added option should be added
> > into each strings to be passed to 'checkopt()' internal function:
> > these are white-space-separated list of un-acceptable option names.
> >
> > The name of new option should be added into multiple strings, because
> > every options can belong to only one category of 'create', 'cleanup',
> > 'delete' or 'list'.
> >
> > In addition of this redundancy, each strings passed to 'checkopt()'
> > are already too long to include new one.
> >
> > This patch refactors option combination check to add new one easily in
> > succeeding patch.
> >
> > New 'checkopt()' takes only one of categories ('cleanup', 'delete' or
> > 'list'), and checks whether option allowed only for other categories
> > is specified or not, if specified category is activated in 'opts'.
> >
> > '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.
> >
> >   - but explicit listing it up can advertise that this ignoring is
> >     intentional
> 
> Yes, this seems fine to me. If you want something to bikeshed (Katsunori
> should verify this):

Thank you for your refining my text, Sean.

I can't find any problems out in refined one.

Should I resend refined one again ? > Pierre-Yves

> ========
> 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 new one.
> 
> This patch refactors option combination check to make it easier to add a
> new option in succeeding patch.
> 
> Now, 'checkopt()' only takes one category ('cleanup', 'delete' or
> 'list'), and checks whether the option allowed for other categories is
> specified 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.
> 
>   - explicit listing it can advertise that ignoring it is intentional
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - June 6, 2014, 5:31 p.m.
On 06/06/2014 06:24 AM, FUJIWARA Katsunori wrote:
> At Thu, 05 Jun 2014 18:59:06 -0500,
> Sean Farley wrote:
>>
>>
>> FUJIWARA Katsunori writes:
>>
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>> # Date 1401977395 -32400
>>> #      Thu Jun 05 23:09:55 2014 +0900
>>> # Node ID c90c042600bcb28108366b8c08a06d9ac3259ab7
>>> # Parent  804a341610d154c8a2a4d3d63e36ff2cf08acc4f
>>> shelve: refactor option combination check to add new one easily
>>>
>>> Before this patch, the name of the newly added option should be added
>>> into each strings to be passed to 'checkopt()' internal function:
>>> these are white-space-separated list of un-acceptable option names.
>>>
>>> The name of new option should be added into multiple strings, because
>>> every options can belong to only one category of 'create', 'cleanup',
>>> 'delete' or 'list'.
>>>
>>> In addition of this redundancy, each strings passed to 'checkopt()'
>>> are already too long to include new one.
>>>
>>> This patch refactors option combination check to add new one easily in
>>> succeeding patch.
>>>
>>> New 'checkopt()' takes only one of categories ('cleanup', 'delete' or
>>> 'list'), and checks whether option allowed only for other categories
>>> is specified or not, if specified category is activated in 'opts'.
>>>
>>> '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.
>>>
>>>    - but explicit listing it up can advertise that this ignoring is
>>>      intentional
>>
>> Yes, this seems fine to me. If you want something to bikeshed (Katsunori
>> should verify this):
>
> Thank you for your refining my text, Sean.
>
> I can't find any problems out in refined one.
>
> Should I resend refined one again ? > Pierre-Yves

Yes please resend a the full fixed series.

Also please replace usage of "succeeding changeset" with "following 
changeset". "succeeding" has an evolution connotation. Those word appear 
a couple of time in your series.
Katsunori FUJIWARA - June 7, 2014, 10:07 a.m.
At Fri, 06 Jun 2014 10:31:01 -0700,
Pierre-Yves David wrote:
> 
> On 06/06/2014 06:24 AM, FUJIWARA Katsunori wrote:
> > At Thu, 05 Jun 2014 18:59:06 -0500,
> > Sean Farley wrote:
> >>
> >>
> >> FUJIWARA Katsunori writes:
> >>
> >>> # HG changeset patch
> >>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> >>> # Date 1401977395 -32400
> >>> #      Thu Jun 05 23:09:55 2014 +0900
> >>> # Node ID c90c042600bcb28108366b8c08a06d9ac3259ab7
> >>> # Parent  804a341610d154c8a2a4d3d63e36ff2cf08acc4f
> >>> shelve: refactor option combination check to add new one easily
> >>>
> >>> Before this patch, the name of the newly added option should be added
> >>> into each strings to be passed to 'checkopt()' internal function:
> >>> these are white-space-separated list of un-acceptable option names.
> >>>
> >>> The name of new option should be added into multiple strings, because
> >>> every options can belong to only one category of 'create', 'cleanup',
> >>> 'delete' or 'list'.
> >>>
> >>> In addition of this redundancy, each strings passed to 'checkopt()'
> >>> are already too long to include new one.
> >>>
> >>> This patch refactors option combination check to add new one easily in
> >>> succeeding patch.
> >>>
> >>> New 'checkopt()' takes only one of categories ('cleanup', 'delete' or
> >>> 'list'), and checks whether option allowed only for other categories
> >>> is specified or not, if specified category is activated in 'opts'.
> >>>
> >>> '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.
> >>>
> >>>    - but explicit listing it up can advertise that this ignoring is
> >>>      intentional
> >>
> >> Yes, this seems fine to me. If you want something to bikeshed (Katsunori
> >> should verify this):
> >
> > Thank you for your refining my text, Sean.
> >
> > I can't find any problems out in refined one.
> >
> > Should I resend refined one again ? > Pierre-Yves
> 
> Yes please resend a the full fixed series.
> 
> Also please replace usage of "succeeding changeset" with "following 
> changeset". "succeeding" has an evolution connotation. Those word appear 
> a couple of time in your series.

I'll use "following" instead of "succeeding".

BTW, "preceding" doesn't have any specific connotation, does it ?  or
is another word appropriate as the antonym of "following" ?

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - July 5, 2014, 9:01 p.m.
On 06/07/2014 12:07 PM, FUJIWARA Katsunori wrote:
> BTW, "preceding" doesn't have any specific connotation, does it ?  or
> is another word appropriate as the antonym of "following" ?

Evolution terminology is:

   Successor - precursor
   Succeed - precede
Katsunori FUJIWARA - July 7, 2014, 10:43 p.m.
At Sat, 05 Jul 2014 23:01:20 +0200,
Pierre-Yves David wrote:
> 
> 
> 
> On 06/07/2014 12:07 PM, FUJIWARA Katsunori wrote:
> > BTW, "preceding" doesn't have any specific connotation, does it ?  or
> > is another word appropriate as the antonym of "following" ?
> 
> Evolution terminology is:
> 
>    Successor - precursor
>    Succeed - precede

OK, I'll use "following <=> previous" or so for patch description,
instead of "succeeding <=> preceding",

----------------------------------------------------------------------
[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'):