Patchwork [2,of,5] registrar: introduce funcregistrarbase to replace funcregistrar

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Jan. 5, 2016, 11:48 a.m.
Message ID <de07b46cc5de1baa86d4.1451994515@feefifofum>
Download mbox | patch
Permalink /patch/12527/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Katsunori FUJIWARA - Jan. 5, 2016, 11:48 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1451994203 -32400
#      Tue Jan 05 20:43:23 2016 +0900
# Node ID de07b46cc5de1baa86d40492941fd56c849311c3
# Parent  e526fa5df73b5263d164bd339251481050755720
registrar: introduce funcregistrarbase to replace funcregistrar

'funcregistrar' puts decorated function into actual table immediately,
and assumes that (3rd party) extension uses it with 'delayregistrar'
to delay registration until uisetup or so.

On the other hand, 'funcregistrarbase' stores all decorated functions
into own internal 'table', and assumes that any implicit/explicit
loading process is executed to put them into actual table.

Such loading process will be added one by one in subsequent patches.

'extrasetup()' is invoked with *args and **kwargs arguments in
'doregister()', but it is defined without them, because this can
reject unknown optional argument at decoration easily, if derived
class doesn't override 'extrasetup()' with any optional arguments.
Yuya Nishihara - Jan. 7, 2016, 2:46 p.m.
On Tue, 05 Jan 2016 20:48:35 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1451994203 -32400
> #      Tue Jan 05 20:43:23 2016 +0900
> # Node ID de07b46cc5de1baa86d40492941fd56c849311c3
> # Parent  e526fa5df73b5263d164bd339251481050755720
> registrar: introduce funcregistrarbase to replace funcregistrar

> 'extrasetup()' is invoked with *args and **kwargs arguments in
> 'doregister()', but it is defined without them, because this can
> reject unknown optional argument at decoration easily, if derived
> class doesn't override 'extrasetup()' with any optional arguments.

Neat.

> +class funcregistrarbase(object):

Let's mark this class and its internal callbacks as private to make sure
that the derived classes should be defined in the registrar.

> +    def __init__(self):
> +        self.table = {}

Perhaps core modules would want to pass a table.

  # revset.py
  predicate = registrar.revsetpredicate(symbols)

If there's no revset.safesymbols, we can do it.

> +    def doregister(self, func, decl, *args, **kwargs):
> +        name = self.getname(decl)
> +
> +        if func.__doc__ and not util.safehasattr(func, '_origdoc'):
> +            doc = func.__doc__.strip()
> +            func._origdoc = doc
> +            if callable(self.formatdoc):
> +                func.__doc__ = self.formatdoc(decl, doc)
> +            else:
> +                # convenient shortcut for simple format
> +                func.__doc__ = self.formatdoc % (decl, doc)

I'm a little shocked that formatdoc() is overloaded as a format string.
I would rather do

  def _formatdoc(self, decl, doc):
      return self._doctemplate % (decl, doc)  # override me if needed

> +    def parsefuncdecl(self, decl):
> +        """Parse function declaration and return the name of function in it
> +        """
> +        i = decl.find('(')
> +        if i > 0:
> +            return decl[:i]
> +        else:
> +            return decl

Is it intentional that i == 0 isn't counted as a function declaration?
"()" is invalid name anyway, but I'm just curious why you've chosen i > 0.
Katsunori FUJIWARA - Jan. 9, 2016, 10:46 a.m.
At Thu, 7 Jan 2016 23:46:02 +0900,
Yuya Nishihara wrote:
> 
> On Tue, 05 Jan 2016 20:48:35 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1451994203 -32400
> > #      Tue Jan 05 20:43:23 2016 +0900
> > # Node ID de07b46cc5de1baa86d40492941fd56c849311c3
> > # Parent  e526fa5df73b5263d164bd339251481050755720
> > registrar: introduce funcregistrarbase to replace funcregistrar
> 
> > 'extrasetup()' is invoked with *args and **kwargs arguments in
> > 'doregister()', but it is defined without them, because this can
> > reject unknown optional argument at decoration easily, if derived
> > class doesn't override 'extrasetup()' with any optional arguments.
> 
> Neat.
> 
> > +class funcregistrarbase(object):
> 
> Let's mark this class and its internal callbacks as private to make sure
> that the derived classes should be defined in the registrar.

I'll do so in revised series.

> > +    def __init__(self):
> > +        self.table = {}
> 
> Perhaps core modules would want to pass a table.
> 
>   # revset.py
>   predicate = registrar.revsetpredicate(symbols)
> 
> If there's no revset.safesymbols, we can do it.

You would assume that passing table directly to decorator can avoid
explicit invocation of extra loading function at the end of own
module, wouldn't you ?  (e.g. templatekw, templatefilters or so)

It is reasonable, but on the other hand, similarity between modules
using this extra loading mechanism seems important, too, IMHO.


> > +    def doregister(self, func, decl, *args, **kwargs):
> > +        name = self.getname(decl)
> > +
> > +        if func.__doc__ and not util.safehasattr(func, '_origdoc'):
> > +            doc = func.__doc__.strip()
> > +            func._origdoc = doc
> > +            if callable(self.formatdoc):
> > +                func.__doc__ = self.formatdoc(decl, doc)
> > +            else:
> > +                # convenient shortcut for simple format
> > +                func.__doc__ = self.formatdoc % (decl, doc)
> 
> I'm a little shocked that formatdoc() is overloaded as a format string.
> I would rather do
> 
>   def _formatdoc(self, decl, doc):
>       return self._doctemplate % (decl, doc)  # override me if needed

I'll do so in revised series.

> > +    def parsefuncdecl(self, decl):
> > +        """Parse function declaration and return the name of function in it
> > +        """
> > +        i = decl.find('(')
> > +        if i > 0:
> > +            return decl[:i]
> > +        else:
> > +            return decl
> 
> Is it intentional that i == 0 isn't counted as a function declaration?
> "()" is invalid name anyway, but I'm just curious why you've chosen i > 0.

Treating "i == 0" as regular name puts "empty name" into table, and it
is never fetched via current tokenizers (isn't it ? :-))

Therefore, this patch treats such declaration as "name for special
internal purpose" (e.g. direct expanding tree for revset alias).

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Jan. 9, 2016, 11:25 a.m.
On Sat, 09 Jan 2016 19:46:34 +0900, FUJIWARA Katsunori wrote:
> At Thu, 7 Jan 2016 23:46:02 +0900,
> Yuya Nishihara wrote:
> > > +    def __init__(self):
> > > +        self.table = {}
> > 
> > Perhaps core modules would want to pass a table.
> > 
> >   # revset.py
> >   predicate = registrar.revsetpredicate(symbols)
> > 
> > If there's no revset.safesymbols, we can do it.
> 
> You would assume that passing table directly to decorator can avoid
> explicit invocation of extra loading function at the end of own
> module, wouldn't you ?  (e.g. templatekw, templatefilters or so)

Yes, it looks ugly to me to call extra setup function by the hosting module
itself.

> It is reasonable, but on the other hand, similarity between modules
> using this extra loading mechanism seems important, too, IMHO.

Eventually we can remove revset.safesymbols and we'll no longer need to
call loadpredicate(None, None, predicate).

> > > +    def parsefuncdecl(self, decl):
> > > +        """Parse function declaration and return the name of function in it
> > > +        """
> > > +        i = decl.find('(')
> > > +        if i > 0:
> > > +            return decl[:i]
> > > +        else:
> > > +            return decl
> > 
> > Is it intentional that i == 0 isn't counted as a function declaration?
> > "()" is invalid name anyway, but I'm just curious why you've chosen i > 0.
> 
> Treating "i == 0" as regular name puts "empty name" into table, and it
> is never fetched via current tokenizers (isn't it ? :-))
> 
> Therefore, this patch treats such declaration as "name for special
> internal purpose" (e.g. direct expanding tree for revset alias).

Ok, though I don't understand the possible use of '(..' name, I'm fine with
it.

Patch

diff --git a/mercurial/registrar.py b/mercurial/registrar.py
--- a/mercurial/registrar.py
+++ b/mercurial/registrar.py
@@ -126,3 +126,85 @@  class delayregistrar(object):
         while self._list:
             func, decorator = self._list.pop(0)
             decorator(func)
+
+class funcregistrarbase(object):
+    """Base of decorator to register a fuction for specific purpose
+
+    This decorator stores decorated functions into own dict 'table'.
+
+    The least derived class can be defined by overriding 'formatdoc',
+    for example::
+
+        class keyword(funcregistrarbase):
+            formatdoc = ":%s: %s"
+
+    This should be used as below:
+
+        keyword = registrar.keyword()
+
+        @keyword('bar')
+        def barfunc(*args, **kwargs):
+            '''Explanation of bar keyword ....
+            '''
+            pass
+
+    In this case:
+
+    - 'barfunc' is stored as 'bar' in 'table' of an instance 'keyword' above
+    - 'barfunc.__doc__' becomes ":bar: Explanation of bar keyword"
+    """
+    def __init__(self):
+        self.table = {}
+
+    def __call__(self, decl, *args, **kwargs):
+        return lambda func: self.doregister(func, decl, *args, **kwargs)
+
+    def doregister(self, func, decl, *args, **kwargs):
+        name = self.getname(decl)
+
+        if func.__doc__ and not util.safehasattr(func, '_origdoc'):
+            doc = func.__doc__.strip()
+            func._origdoc = doc
+            if callable(self.formatdoc):
+                func.__doc__ = self.formatdoc(decl, doc)
+            else:
+                # convenient shortcut for simple format
+                func.__doc__ = self.formatdoc % (decl, doc)
+
+        self.table[name] = func
+        self.extrasetup(name, func, *args, **kwargs)
+
+        return func
+
+    def parsefuncdecl(self, decl):
+        """Parse function declaration and return the name of function in it
+        """
+        i = decl.find('(')
+        if i > 0:
+            return decl[:i]
+        else:
+            return decl
+
+    def getname(self, decl):
+        """Return the name of the registered function from decl
+
+        Derived class should override this, if it allows more
+        descriptive 'decl' string than just a name.
+        """
+        return decl
+
+    def formatdoc(self, decl, doc):
+        """Return formatted document of the registered function for help
+
+        'doc' is '__doc__.strip()' of the registered function.
+
+        If this is overridden by non-callable object in derived class,
+        such value is treated as "format string" and used to format
+        document by 'self.formatdoc % (decl, doc)' for convenience.
+        """
+        raise NotImplementedError()
+
+    def extrasetup(self, name, func):
+        """Execute exra setup for registered function, if needed
+        """
+        pass