[PATCH] fancyopts: allow all callable as default parameter value

Jordi GutiƩrrez Hermoso jordigh at octave.org
Thu Jun 11 10:18:32 CDT 2015


Hi, looks like you figured out how to send patches to hg. Great!

On Thu, 2015-06-11 at 09:04 -0400, shiyao.ma wrote:
> # HG changeset patch
> # User introom <i at introo.me>
> # Date 1434027264 14400
> #      Thu Jun 11 08:54:24 2015 -0400
> # Node ID 54fb12cb448edd26a5334940af7b33e4b4414615
> # Parent  142fb8767743ff6b1c02a6ba01c5935f8dcf422d
> fancyopts: allow all callable as default parameter value
> 
> The current fancyopts allows function as default parameter value
> but not other callables.
> By supporting other callables, we can have the benefits of e.g.,
> custom __str__ method, which will be printed by 'hg help' as
> the default value.

Okay, but...

> 
> diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
> --- a/mercurial/fancyopts.py
> +++ b/mercurial/fancyopts.py
> @@ -104,19 +104,20 @@
>      for opt, val in opts:
>          name = argmap[opt]
>          obj = defmap[name]
> +        t = type(obj)
>          if callable(obj):
>              state[name] = defmap[name](val)
> -        elif isinstance(obj, int):
> +        elif t is type(1):

I don't see what these changes are doing or what do they have to do
with your goal as stated in the commit message. Under what
circumstance will `isintance(obj, int)` return true but `type(obj) is
int` return false?

Also, why are you using `type(1)` instead of `int`?

The same question applies to all subsequent changes. I don't
understand how your patch changes anything at all. If you are sure
that your patch is indeed correct, can you please resend with a longer
explanation in your commit message? Use `--flag V2` in `hg email` to
resend.

>              try:
>                  state[name] = int(val)
>              except ValueError:
>                  raise util.Abort(_('invalid value %r for option %s, '
>                                     'expected int') % (val, opt))
> -        elif isinstance(obj, basestring):
> +        elif t is type(''):
>              state[name] = val
> -        elif isinstance(obj, list):
> +        elif t is type([]):
>              state[name].append(val)
> -        elif obj is None or isinstance(obj, bool):
> +        elif t is type(None) or t is type(False):
>              state[name] = True
>  
>      # return unparsed args
> 






More information about the Mercurial-devel mailing list