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 Superseded
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
phabricator - Feb. 21, 2018, 4:18 a.m.
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
phabricator - Feb. 21, 2018, 5:42 a.m.
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
phabricator - Feb. 22, 2018, 3:26 a.m.
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
phabricator - Feb. 22, 2018, 3:27 a.m.
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
phabricator - Feb. 22, 2018, 3:56 a.m.
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
phabricator - Feb. 22, 2018, 1:22 p.m.
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

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