D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py

indygreg (Gregory Szorc) phabricator at mercurial-scm.org
Wed Feb 21 00:42:41 EST 2018


indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I like where this is going.
  
  Out of curiosity, do you think it would be possible to implement an option that behaved like a boolean when given in isolation but also optionally accepted a value? My use case is I want `hg serve --open` to automatically open a web browser pointing at the started server and `hg serve --open chrome` to open Chrome instead of my default web browser. I'm not sure if that's a good idea to implement in the parser though. It could possibly lead to ambiguous argument parsing.
  
  Anyway, I'd be happy to queue this. But I'd like to hear your reply about my review comments first in case you feel like improving this code as part of the refactor.

INLINE COMMENTS

> fancyopts.py:224-225
> +    def _isboolopt(self):
> +        t = type(self.defaultvalue)
> +        return t is type(False) or t is type(None)
> +

Can we do `isinstance(self.defaultvalue, (bool, types.NoneType))` instead?

> fancyopts.py:231-232
> +class _callableopt(customopt):
> +    def __init__(self, callable):
> +        self.callable = callable
> +        super(_callableopt, self).__init__(None)

`callable` is a built-in symbol in Python and should not be redefined.

> fancyopts.py:258-261
> +    elif t is type([]):
> +        return _listopt(default[:])
> +    elif t is type(1):
> +        return _intopt(default)

`isinstance()` is preferred here. Although the integer check could be a bit wonky if Python 2's `long` ever comes into play.

> fancyopts.py:313-317
> -        elif t is type(''):
> -            state[name] = val
> -        elif t is type([]):
> -            state[name].append(val)
> -        elif t is type(None) or t is type(False):

Oh, your code was copied from here. I suppose it is mostly OK then. Although bringing this into modernity as part of the refactor wouldn't hurt...

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2090

To: dploch, #hg-reviewers, durin42, indygreg
Cc: durin42, indygreg, mercurial-devel


More information about the Mercurial-devel mailing list