D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
yuja added inline comments. INLINE COMMENTS > fancyopts.py:258 > +elif isinstance(default, list): > +return _listopt(default[:]) > +elif type(default) is type(1): Perhaps it's safer to make `defaultvalue()` a function returning a copy of default, instead of passing a copy to `_listopt()`. If we make `_listopt` public and start putting it into the static command table, things will go wrong. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2090 To: dploch, #hg-reviewers, durin42, indygreg Cc: yuja, durin42, indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
This revision was automatically updated to reflect the committed changes.
dploch marked 2 inline comments as done.
Closed by commit rHG2ed36fec5321: fancyopts: add support for custom multi-arg
opts in fancyopts.py (authored by dploch, committed by ).
CHANGED PRIOR TO COMMIT
https://phab.mercurial-scm.org/D2090?vs=5981=5984#toc
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D2090?vs=5981=5984
REVISION DETAIL
https://phab.mercurial-scm.org/D2090
AFFECTED FILES
mercurial/fancyopts.py
CHANGE DETAILS
diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
--- a/mercurial/fancyopts.py
+++ b/mercurial/fancyopts.py
@@ -7,7 +7,9 @@
from __future__ import absolute_import
+import abc
import functools
+import types
from .i18n import _
from . import (
@@ -201,6 +203,64 @@
parsedargs.extend(args[pos:])
return parsedopts, parsedargs
+class customopt(object):
+"""Manage defaults and mutations for any type of opt."""
+
+__metaclass__ = abc.ABCMeta
+
+def __init__(self, defaultvalue):
+self.defaultvalue = defaultvalue
+
+def _isboolopt(self):
+return False
+
+@abc.abstractmethod
+def newstate(self, oldstate, newparam, abort):
+"""Adds newparam to oldstate and returns the new state.
+
+On failure, abort can be called with a string error message."""
+
+class _simpleopt(customopt):
+def _isboolopt(self):
+return isinstance(self.defaultvalue, (bool, types.NoneType))
+
+def newstate(self, oldstate, newparam, abort):
+return newparam
+
+class _callableopt(customopt):
+def __init__(self, callablefn):
+self.callablefn = callablefn
+super(_callableopt, self).__init__(None)
+
+def newstate(self, oldstate, newparam, abort):
+return self.callablefn(newparam)
+
+class _listopt(customopt):
+def newstate(self, oldstate, newparam, abort):
+oldstate.append(newparam)
+return oldstate
+
+class _intopt(customopt):
+def newstate(self, oldstate, newparam, abort):
+try:
+return int(newparam)
+except ValueError:
+abort(_('expected int'))
+
+def _defaultopt(default):
+"""Returns a default opt implementation, given a default value."""
+
+if isinstance(default, customopt):
+return default
+elif callable(default):
+return _callableopt(default)
+elif isinstance(default, list):
+return _listopt(default[:])
+elif type(default) is type(1):
+return _intopt(default)
+else:
+return _simpleopt(default)
+
def fancyopts(args, options, state, gnu=False, early=False, optaliases=None):
"""
read args, parse options, and store options in state
@@ -220,6 +280,7 @@
list - parameter string is added to a list
integer - parameter strings is stored as int
function - call function with parameter
+ customopt - subclass of 'customopt'
optaliases is a mapping from a canonical option name to a list of
additional long options. This exists for preserving backward compatibility
@@ -250,18 +311,13 @@
argmap['-' + short] = name
for n in onames:
argmap['--' + n] = name
-defmap[name] = default
+defmap[name] = _defaultopt(default)
# copy defaults to state
-if isinstance(default, list):
-state[name] = default[:]
-elif callable(default):
-state[name] = None
-else:
-state[name] = default
+state[name] = defmap[name].defaultvalue
# does it take a parameter?
-if not (default is None or default is True or default is False):
+if not defmap[name]._isboolopt():
if short:
short += ':'
onames = [n + '=' for n in onames]
@@ -301,21 +357,13 @@
boolval = False
name = argmap[opt]
obj = defmap[name]
-t = type(obj)
-if callable(obj):
-state[name] = defmap[name](val)
-elif t is type(1):
-try:
-state[name] = int(val)
-except ValueError:
-raise error.Abort(_('invalid value %r for option %s, '
- 'expected int') % (val, opt))
-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):
+if obj._isboolopt():
state[name] = boolval
+else:
+def abort(s):
+raise error.Abort(
+_('invalid value %r for option %s, %s') % (val, opt, s))
+state[name] = defmap[name].newstate(state[name], val, abort)
# return unparsed args
return args
To: dploch, #hg-reviewers, durin42, indygreg
Cc: durin42, indygreg, mercurial-devel
___
Mercurial-devel mailing list
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
indygreg accepted this revision. indygreg added a comment. This revision is now accepted and ready to land. Thanks for following up with the style changes! INLINE COMMENTS > dploch wrote in fancyopts.py:258-261 > It's actually wonkier than expected... :) That's some PHP/JavaScript wonkiness :/ I liked the old form for its brevity and will change this as part of landing. 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
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
durin42 added a comment. In https://phab.mercurial-scm.org/D2090#39120, @dploch wrote: > In https://phab.mercurial-scm.org/D2090#38775, @indygreg wrote: > > > 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. > > > It feels like a bad idea. If we want the parsing to be unambiguous, then '--open' must always come at the end of the command line if the default option is desired. If there were a separate flag with the same semantics, you couldn't specify both: 'hg serve --open --foo' would pass "--foo" as the argument to --open. > > If we stipulate that the optional argument only counts as an argument if it doesn't look like a flag (^-(-)?[^\d-].*$), it sort of works, but that feels pretty messy. I agree, it's too hard to understand the parsing rules if we allow this. I actually explored this a bit with negating boolean flags and it's just Too Messy in my opinion. 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
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
dploch added a comment. In https://phab.mercurial-scm.org/D2090#38775, @indygreg wrote: > 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. It feels like a bad idea. If we want the parsing to be unambiguous, then '--open' must always come at the end of the command line if the default option is desired. If there were a separate flag with the same semantics, you couldn't specify both: 'hg serve --open --foo' would pass "--foo" as the argument to --open. If we stipulate that the optional argument only counts as an argument if it doesn't look like a flag (^-(-)?[^\d-].*$), it sort of works, but that feels pretty messy. INLINE COMMENTS > indygreg wrote in fancyopts.py:258-261 > `isinstance()` is preferred here. Although the integer check could be a bit > wonky if Python 2's `long` ever comes into play. It's actually wonkier than expected... :) > indygreg wrote in fancyopts.py:313-317 > 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... Sure, modernized. 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
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
dploch updated this revision to Diff 5981.
dploch marked 4 inline comments as done.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D2090?vs=5330=5981
REVISION DETAIL
https://phab.mercurial-scm.org/D2090
AFFECTED FILES
mercurial/fancyopts.py
CHANGE DETAILS
diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
--- a/mercurial/fancyopts.py
+++ b/mercurial/fancyopts.py
@@ -7,7 +7,9 @@
from __future__ import absolute_import
+import abc
import functools
+import types
from .i18n import _
from . import (
@@ -201,6 +203,65 @@
parsedargs.extend(args[pos:])
return parsedopts, parsedargs
+class customopt(object):
+"""Manage defaults and mutations for any type of opt."""
+
+__metaclass__ = abc.ABCMeta
+
+def __init__(self, defaultvalue):
+self.defaultvalue = defaultvalue
+
+def _isboolopt(self):
+return False
+
+@abc.abstractmethod
+def newstate(self, oldstate, newparam, abort):
+"""Adds newparam to oldstate and returns the new state.
+
+On failure, abort can be called with a string error message."""
+
+class _simpleopt(customopt):
+def _isboolopt(self):
+return isinstance(self.defaultvalue, (bool, types.NoneType))
+
+def newstate(self, oldstate, newparam, abort):
+return newparam
+
+class _callableopt(customopt):
+def __init__(self, callablefn):
+self.callablefn = callablefn
+super(_callableopt, self).__init__(None)
+
+def newstate(self, oldstate, newparam, abort):
+return self.callablefn(newparam)
+
+class _listopt(customopt):
+def newstate(self, oldstate, newparam, abort):
+oldstate.append(newparam)
+return oldstate
+
+class _intopt(customopt):
+def newstate(self, oldstate, newparam, abort):
+try:
+return int(newparam)
+except ValueError:
+abort(_('expected int'))
+
+def _defaultopt(default):
+"""Returns a default opt implementation, given a default value."""
+
+if isinstance(default, customopt):
+return default
+elif callable(default):
+return _callableopt(default)
+elif isinstance(default, list):
+return _listopt(default[:])
+# isinstance(True, int) returns True for some reason.
+elif isinstance(default, (int, long)) and not isinstance(default, bool):
+return _intopt(default)
+else:
+return _simpleopt(default)
+
def fancyopts(args, options, state, gnu=False, early=False, optaliases=None):
"""
read args, parse options, and store options in state
@@ -220,6 +281,7 @@
list - parameter string is added to a list
integer - parameter strings is stored as int
function - call function with parameter
+ customopt - subclass of 'customopt'
optaliases is a mapping from a canonical option name to a list of
additional long options. This exists for preserving backward compatibility
@@ -250,18 +312,13 @@
argmap['-' + short] = name
for n in onames:
argmap['--' + n] = name
-defmap[name] = default
+defmap[name] = _defaultopt(default)
# copy defaults to state
-if isinstance(default, list):
-state[name] = default[:]
-elif callable(default):
-state[name] = None
-else:
-state[name] = default
+state[name] = defmap[name].defaultvalue
# does it take a parameter?
-if not (default is None or default is True or default is False):
+if not defmap[name]._isboolopt():
if short:
short += ':'
onames = [n + '=' for n in onames]
@@ -301,21 +358,13 @@
boolval = False
name = argmap[opt]
obj = defmap[name]
-t = type(obj)
-if callable(obj):
-state[name] = defmap[name](val)
-elif t is type(1):
-try:
-state[name] = int(val)
-except ValueError:
-raise error.Abort(_('invalid value %r for option %s, '
- 'expected int') % (val, opt))
-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):
+if obj._isboolopt():
state[name] = boolval
+else:
+def abort(s):
+raise error.Abort(
+_('invalid value %r for option %s, %s') % (val, opt, s))
+state[name] = defmap[name].newstate(state[name], val, abort)
# return unparsed args
return args
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
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
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
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
dploch added a comment. Friendly ping! This is my first commit so I'm not sure if more information or changes are expected; please let me know if there's anything I'm missing. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2090 To: dploch, #hg-reviewers, durin42 Cc: durin42, indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
durin42 accepted this revision as: durin42. durin42 added a comment. I _really_ like where this is headed, but will refrain from queueing for now since it's a bit of a conflict of interest. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2090 To: dploch, #hg-reviewers, durin42 Cc: durin42, indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
dploch added a comment. In https://phab.mercurial-scm.org/D2090#34906, @indygreg wrote: > The fancyopts code is some of the oldest in Mercurial. We've been wanting to rewrite it for a while. This patch seems like an interesting and more powerful direction to take the parser. > > Out of curiosity, do you have an intended use case in mind? Will that use case be in Mercurial itself or is this for an extension? I didn't have a use case for Mercurial itself in mind, but I wouldn't be surprised if there was one. My intended use case is the 'csv' flag example in the commit description: We have a lot of flags in our internal extensions that require the ["alice,bob", "charlie"] -> ["alice", "bob", "charlie"] behavior, so it would be really nice to be able to declare a shareable customopt for these instead of needing to individually wrap every applicable `opts['flag']` lookup. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2090 To: dploch, #hg-reviewers Cc: indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
indygreg added a comment. The fancyopts code is some of the oldest in Mercurial. We've been wanting to rewrite it for a while. This patch seems like an interesting and more powerful direction to take the parser. Out of curiosity, do you have an intended use case in mind? Will that use case be in Mercurial itself or is this for an extension? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2090 To: dploch, #hg-reviewers Cc: indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D2090: fancyopts: add support for custom multi-arg opts in fancyopts.py
dploch created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
This allows for more complex multi-arg opt logic, such as "--sum 1 --sum 2"
-> 3, "--csv alice,bob --csv charlie" -> ["alice","bob","charlie"]. The
current support for callables is insufficient for this.
This is done by introducing a 'customopt' class which can be extended for
more powerful opts logic. All existing opt-types are converted to use this
class, simplifying the fancyopts() logic.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D2090
AFFECTED FILES
mercurial/fancyopts.py
CHANGE DETAILS
diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
--- a/mercurial/fancyopts.py
+++ b/mercurial/fancyopts.py
@@ -7,6 +7,7 @@
from __future__ import absolute_import
+import abc
import functools
from .i18n import _
@@ -201,6 +202,66 @@
parsedargs.extend(args[pos:])
return parsedopts, parsedargs
+class customopt(object):
+"""Manage defaults and mutations for any type of opt."""
+
+__metaclass__ = abc.ABCMeta
+
+def __init__(self, defaultvalue):
+self.defaultvalue = defaultvalue
+
+def _isboolopt(self):
+return False
+
+@abc.abstractmethod
+def newstate(self, oldstate, newparam, abort):
+"""Adds newparam to oldstate and returns the new state.
+
+On failure, abort can be called with a string error message."""
+
+class _simpleopt(customopt):
+def _isboolopt(self):
+t = type(self.defaultvalue)
+return t is type(False) or t is type(None)
+
+def newstate(self, oldstate, newparam, abort):
+return newparam
+
+class _callableopt(customopt):
+def __init__(self, callable):
+self.callable = callable
+super(_callableopt, self).__init__(None)
+
+def newstate(self, oldstate, newparam, abort):
+return self.callable(newparam)
+
+class _listopt(customopt):
+def newstate(self, oldstate, newparam, abort):
+oldstate.append(newparam)
+return oldstate
+
+class _intopt(customopt):
+def newstate(self, oldstate, newparam, abort):
+try:
+return int(newparam)
+except ValueError:
+abort(_('expected int'))
+
+def _defaultopt(default):
+"""Returns a default opt implementation, given a default value."""
+
+t = type(default)
+if isinstance(default, customopt):
+return default
+elif callable(default):
+return _callableopt(default)
+elif t is type([]):
+return _listopt(default[:])
+elif t is type(1):
+return _intopt(default)
+else:
+return _simpleopt(default)
+
def fancyopts(args, options, state, gnu=False, early=False, optaliases=None):
"""
read args, parse options, and store options in state
@@ -220,6 +281,7 @@
list - parameter string is added to a list
integer - parameter strings is stored as int
function - call function with parameter
+ customopt - subclass of 'customopt'
optaliases is a mapping from a canonical option name to a list of
additional long options. This exists for preserving backward compatibility
@@ -250,18 +312,13 @@
argmap['-' + short] = name
for n in onames:
argmap['--' + n] = name
-defmap[name] = default
+defmap[name] = _defaultopt(default)
# copy defaults to state
-if isinstance(default, list):
-state[name] = default[:]
-elif callable(default):
-state[name] = None
-else:
-state[name] = default
+state[name] = defmap[name].defaultvalue
# does it take a parameter?
-if not (default is None or default is True or default is False):
+if not defmap[name]._isboolopt():
if short:
short += ':'
onames = [n + '=' for n in onames]
@@ -301,21 +358,13 @@
boolval = False
name = argmap[opt]
obj = defmap[name]
-t = type(obj)
-if callable(obj):
-state[name] = defmap[name](val)
-elif t is type(1):
-try:
-state[name] = int(val)
-except ValueError:
-raise error.Abort(_('invalid value %r for option %s, '
- 'expected int') % (val, opt))
-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):
+if obj._isboolopt():
state[name] = boolval
+else:
+def abort(s):
+raise error.Abort(
+_('invalid value %r for option %s, %s') % (val, opt, s))
+state[name] = defmap[name].newstate(state[name], val, abort)
# return unparsed args
return args
To: dploch, #hg-reviewers
Cc: mercurial-devel
