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

login
register
mail settings
Submitter phabricator
Date Feb. 8, 2018, 1:38 a.m.
Message ID <differential-rev-PHID-DREV-wbfxkatgrsoqia72jl4j-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27464/
State New
Headers show

Comments

phabricator - Feb. 8, 2018, 1:38 a.m.
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




To: dploch, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 8, 2018, 2:35 a.m.
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
phabricator - Feb. 8, 2018, 2:53 a.m.
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
phabricator - Feb. 15, 2018, 1:42 a.m.
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

Patch

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