Patchwork [2,of,6] parser: add stub class that will host alias parsing and expansion

login
register
mail settings
Submitter Yuya Nishihara
Date April 1, 2016, 3:37 p.m.
Message ID <2cad9bec8edb7d4a2cd9.1459525028@mimosa>
Download mbox | patch
Permalink /patch/14223/
State Superseded
Commit 475dad3432fd5826bfa674192ea62ea75744bd34
Headers show

Comments

Yuya Nishihara - April 1, 2016, 3:37 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1456734174 -32400
#      Mon Feb 29 17:22:54 2016 +0900
# Node ID 2cad9bec8edb7d4a2cd9e317a942d9098e1eda82
# Parent  d30389cc8809d79f9afa1a52275546c816398ab0
parser: add stub class that will host alias parsing and expansion

This class will keep syntax rules that are necessary to parse and expand
aliases. The implementations will be extracted from the revset module. In
order to make the porting easier, this class keeps parsedecl and parsedefn
separately. These functions will be unified later.

The following public functions will be added:

  rules.build(decl, defn) -> aliasobj
    parse decl and defn into an object that keeps alias name, arguments
    and replacement tree.
  rules.buildmap(aliasitems) -> aliasdict
    helper to build() a dict of alias objects from a list of (decl, defn)
  rules.expand(aliasdict, tree) -> tree
    expand aliases in tree recursively

Because these functions aren't introduced by this series, there would remain
a couple of wrapper functions in the revset module. These ugly wrappers
should be eliminated by the next series.
Pierre-Yves David - April 1, 2016, 9:22 p.m.
On 04/01/2016 08:37 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1456734174 -32400
> #      Mon Feb 29 17:22:54 2016 +0900
> # Node ID 2cad9bec8edb7d4a2cd9e317a942d9098e1eda82
> # Parent  d30389cc8809d79f9afa1a52275546c816398ab0
> parser: add stub class that will host alias parsing and expansion
>
> This class will keep syntax rules that are necessary to parse and expand
> aliases. The implementations will be extracted from the revset module. In
> order to make the porting easier, this class keeps parsedecl and parsedefn
> separately. These functions will be unified later.
>
> The following public functions will be added:
>
>    rules.build(decl, defn) -> aliasobj
>      parse decl and defn into an object that keeps alias name, arguments
>      and replacement tree.
>    rules.buildmap(aliasitems) -> aliasdict
>      helper to build() a dict of alias objects from a list of (decl, defn)
>    rules.expand(aliasdict, tree) -> tree
>      expand aliases in tree recursively
>
> Because these functions aren't introduced by this series, there would remain
> a couple of wrapper functions in the revset module. These ugly wrappers
> should be eliminated by the next series.
>
> diff --git a/mercurial/parser.py b/mercurial/parser.py
> --- a/mercurial/parser.py
> +++ b/mercurial/parser.py
> @@ -228,3 +228,28 @@ def parseerrordetail(inst):
>           return _('at %s: %s') % (inst.args[1], inst.args[0])
>       else:
>           return inst.args[0]
> +
> +class aliasrules(object):
> +    """Parsing and expansion rule set of aliases
> +
> +    This class supports aliases of symbol and function-call styles::
> +
> +        # decl = defn
> +        h = heads(default)
> +        b($1) = ancestors($1) - ancestors(default)
> +
> +    - ``section`` is typically a config section name, which will be included
> +      in error messages
> +    - ``parsedecl(spec)`` parses an alias name and arguments
> +    - ``parsedefn(spec)`` parses an alias definition
> +    - ``getlist(tree)`` extracts a list of arguments from parsed tree
> +    """

So this class is an helper that will be used by both template alias and 
revset alias. We should probably mention that.

As far as I understand (so, not too much), this will be used in 2 places 
with always the same two set of arguments. One for template and one for 
revset.

Why don't we just you inheritance here? With an abstract "aliasrules" 
class and two concrete "revsetaliasrules" and "templatealiasrules"?

> +    def __init__(self, section, parsedecl, parsedefn, getlist,
> +                 symbolnode='symbol', funcnode='func'):
> +        self._section = section
> +        self._parsedecl = parsedecl
> +        self._parsedefn = parsedefn
> +        self._getlist = getlist
> +        self._symbolnode = symbolnode
> +        self._funcnode = funcnode

I'm not sure what these symbol and func node means here.
Yuya Nishihara - April 2, 2016, 2:55 a.m.
On Fri, 1 Apr 2016 14:22:44 -0700, Pierre-Yves David wrote:
> On 04/01/2016 08:37 AM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1456734174 -32400
> > #      Mon Feb 29 17:22:54 2016 +0900
> > # Node ID 2cad9bec8edb7d4a2cd9e317a942d9098e1eda82
> > # Parent  d30389cc8809d79f9afa1a52275546c816398ab0
> > parser: add stub class that will host alias parsing and expansion
> >
> > This class will keep syntax rules that are necessary to parse and expand
> > aliases. The implementations will be extracted from the revset module. In
> > order to make the porting easier, this class keeps parsedecl and parsedefn
> > separately. These functions will be unified later.
> >
> > The following public functions will be added:
> >
> >    rules.build(decl, defn) -> aliasobj
> >      parse decl and defn into an object that keeps alias name, arguments
> >      and replacement tree.
> >    rules.buildmap(aliasitems) -> aliasdict
> >      helper to build() a dict of alias objects from a list of (decl, defn)
> >    rules.expand(aliasdict, tree) -> tree
> >      expand aliases in tree recursively
> >
> > Because these functions aren't introduced by this series, there would remain
> > a couple of wrapper functions in the revset module. These ugly wrappers
> > should be eliminated by the next series.
> >
> > diff --git a/mercurial/parser.py b/mercurial/parser.py
> > --- a/mercurial/parser.py
> > +++ b/mercurial/parser.py
> > @@ -228,3 +228,28 @@ def parseerrordetail(inst):
> >           return _('at %s: %s') % (inst.args[1], inst.args[0])
> >       else:
> >           return inst.args[0]
> > +
> > +class aliasrules(object):
> > +    """Parsing and expansion rule set of aliases
> > +
> > +    This class supports aliases of symbol and function-call styles::
> > +
> > +        # decl = defn
> > +        h = heads(default)
> > +        b($1) = ancestors($1) - ancestors(default)
> > +
> > +    - ``section`` is typically a config section name, which will be included
> > +      in error messages
> > +    - ``parsedecl(spec)`` parses an alias name and arguments
> > +    - ``parsedefn(spec)`` parses an alias definition
> > +    - ``getlist(tree)`` extracts a list of arguments from parsed tree
> > +    """  
> 
> So this class is an helper that will be used by both template alias and 
> revset alias. We should probably mention that.

and by fileset alias, perhaps. I'll update the docstring as follows:

  This is a helper for fileset/revset/template aliases. It supports alias
  expansion of symbol and funciton-call styles::

> As far as I understand (so, not too much), this will be used in 2 places
> with always the same two set of arguments. One for template and one for
> revset.
> 
> Why don't we just you inheritance here? With an abstract "aliasrules"
> class and two concrete "revsetaliasrules" and "templatealiasrules"?

I tried that in draft version, but I prefer not to. If "aliasrules" is an
abstract class, its instance would have no state at all because all
customizations would be done statically.

  class revsetaliasrules(aliasrules):
      def __init__(self):
          # no variable

      def _getlist(self, tree):
          return getlist(tree)

      def _parsedecl(self, decl):
          return _parse(decl)
      ...

So it isn't necessary to be a class. This isn't a strong reasoning, but
I'm not a big fan of using a class hierarchy just to hook functions.

> > +    def __init__(self, section, parsedecl, parsedefn, getlist,
> > +                 symbolnode='symbol', funcnode='func'):
> > +        self._section = section
> > +        self._parsedecl = parsedecl
> > +        self._parsedefn = parsedefn
> > +        self._getlist = getlist
> > +        self._symbolnode = symbolnode
> > +        self._funcnode = funcnode  
> 
> I'm not sure what these symbol and func node means here.

They are tags how symbols and functions are marked.

  ('symbol', _)
  ('func', _, _)

Patch

diff --git a/mercurial/parser.py b/mercurial/parser.py
--- a/mercurial/parser.py
+++ b/mercurial/parser.py
@@ -228,3 +228,28 @@  def parseerrordetail(inst):
         return _('at %s: %s') % (inst.args[1], inst.args[0])
     else:
         return inst.args[0]
+
+class aliasrules(object):
+    """Parsing and expansion rule set of aliases
+
+    This class supports aliases of symbol and function-call styles::
+
+        # decl = defn
+        h = heads(default)
+        b($1) = ancestors($1) - ancestors(default)
+
+    - ``section`` is typically a config section name, which will be included
+      in error messages
+    - ``parsedecl(spec)`` parses an alias name and arguments
+    - ``parsedefn(spec)`` parses an alias definition
+    - ``getlist(tree)`` extracts a list of arguments from parsed tree
+    """
+
+    def __init__(self, section, parsedecl, parsedefn, getlist,
+                 symbolnode='symbol', funcnode='func'):
+        self._section = section
+        self._parsedecl = parsedecl
+        self._parsedefn = parsedefn
+        self._getlist = getlist
+        self._symbolnode = symbolnode
+        self._funcnode = funcnode