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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
