Patchwork [1,of,5] dispatch: make loading extra information from extension extensible

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Jan. 5, 2016, 11:48 a.m.
Message ID <e526fa5df73b5263d164.1451994514@feefifofum>
Download mbox | patch
Permalink /patch/12526/
State Superseded
Commit 73905484ef7054d63890cc19552d64f86b26e6cb
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 e526fa5df73b5263d164bd339251481050755720
# Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
dispatch: make loading extra information from extension extensible

This patch makes loading extra information from extension module at
dispatching extensible. This assumes registration of new function like
below, for example:

  - revset predicate
  - fileset predicate
  - template keyword
  - template filter
  - template function
  - internal merge tool
  - web command

This patch requires not loader function itself but container module
and the name of it, because listing loader function directly up
implies loading module of it, even if it isn't used at runtime.
Yuya Nishihara - Jan. 7, 2016, 1:54 p.m.
On Tue, 05 Jan 2016 20:48:34 +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 e526fa5df73b5263d164bd339251481050755720
> # Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
> dispatch: make loading extra information from extension extensible
> 
> This patch makes loading extra information from extension module at
> dispatching extensible. This assumes registration of new function like
> below, for example:
> 
>   - revset predicate
>   - fileset predicate
>   - template keyword
>   - template filter
>   - template function
>   - internal merge tool
>   - web command
> 
> This patch requires not loader function itself but container module
> and the name of it, because listing loader function directly up
> implies loading module of it, even if it isn't used at runtime.
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -749,6 +749,24 @@ def _checkshellalias(lui, ui, args, prec
>      restorecommands()
>  
>  _loaded = set()
> +
> +def _loadcmdtable(ui, name, cmdtable):
> +    overrides = [cmd for cmd in cmdtable if cmd in commands.table]
> +    if overrides:
> +        ui.warn(_("extension '%s' overrides commands: %s\n")
> +                % (name, " ".join(overrides)))
> +    commands.table.update(cmdtable)
> +
> +# list of (objname, loadermod, loadername) tuple:
> +# - objname is the name of an object in extension module, from which
> +#   extra information is loaded
> +# - loadermod is the module where loader is placed
> +# - loadername is the name of the function, which takes (ui, objname,
> +#   obj) arguments
> +extraloaders = [
> +    ('cmdtable', None, None), # this is just a place holder for 'cmdtable'

No special case would be necessary if the commands module had loadcmdtable().
(or updatecmdtable(), registercmdtable(), ... just bikeshedding ;)

>      # (uisetup and extsetup are handled in extensions.loadall)
>  
> +    loadercache = {'cmdtable': _loadcmdtable}

And this cache will no longer be necessary as getattr() should be fast.

>      for name, module in exts:
> -        cmdtable = getattr(module, 'cmdtable', {})
> -        overrides = [cmd for cmd in cmdtable if cmd in commands.table]
> -        if overrides:
> -            ui.warn(_("extension '%s' overrides commands: %s\n")
> -                    % (name, " ".join(overrides)))
> -        commands.table.update(cmdtable)
> +        for objname, loadermod, loadername in extraloaders:
> +            if util.safehasattr(module, objname):

Nit: you can use getattr(module, objname, None).

> +                if objname not in loadercache:
> +                    loadercache[objname] = getattr(loadermod, loadername)
> +                loader = loadercache[objname]
> +                loader(ui, name, getattr(module, objname))
Sean Farley - Jan. 7, 2016, 8 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Tue, 05 Jan 2016 20:48:34 +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 e526fa5df73b5263d164bd339251481050755720
>> # Parent  b8405d739149cdd6d8d9bd5e3dd2ad8487b1f09a
>> dispatch: make loading extra information from extension extensible
>> 
>> This patch makes loading extra information from extension module at
>> dispatching extensible. This assumes registration of new function like
>> below, for example:
>> 
>>   - revset predicate
>>   - fileset predicate
>>   - template keyword
>>   - template filter
>>   - template function
>>   - internal merge tool
>>   - web command
>> 
>> This patch requires not loader function itself but container module
>> and the name of it, because listing loader function directly up
>> implies loading module of it, even if it isn't used at runtime.
>> 
>> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>> --- a/mercurial/dispatch.py
>> +++ b/mercurial/dispatch.py
>> @@ -749,6 +749,24 @@ def _checkshellalias(lui, ui, args, prec
>>      restorecommands()
>>  
>>  _loaded = set()
>> +
>> +def _loadcmdtable(ui, name, cmdtable):
>> +    overrides = [cmd for cmd in cmdtable if cmd in commands.table]
>> +    if overrides:
>> +        ui.warn(_("extension '%s' overrides commands: %s\n")
>> +                % (name, " ".join(overrides)))
>> +    commands.table.update(cmdtable)
>> +
>> +# list of (objname, loadermod, loadername) tuple:
>> +# - objname is the name of an object in extension module, from which
>> +#   extra information is loaded
>> +# - loadermod is the module where loader is placed
>> +# - loadername is the name of the function, which takes (ui, objname,
>> +#   obj) arguments
>> +extraloaders = [
>> +    ('cmdtable', None, None), # this is just a place holder for 'cmdtable'
>
> No special case would be necessary if the commands module had loadcmdtable().
> (or updatecmdtable(), registercmdtable(), ... just bikeshedding ;)
>
>>      # (uisetup and extsetup are handled in extensions.loadall)
>>  
>> +    loadercache = {'cmdtable': _loadcmdtable}
>
> And this cache will no longer be necessary as getattr() should be fast.
>
>>      for name, module in exts:
>> -        cmdtable = getattr(module, 'cmdtable', {})
>> -        overrides = [cmd for cmd in cmdtable if cmd in commands.table]
>> -        if overrides:
>> -            ui.warn(_("extension '%s' overrides commands: %s\n")
>> -                    % (name, " ".join(overrides)))
>> -        commands.table.update(cmdtable)
>> +        for objname, loadermod, loadername in extraloaders:
>> +            if util.safehasattr(module, objname):
>
> Nit: you can use getattr(module, objname, None).

Should we remove util.safehasattr now?
Yuya Nishihara - Jan. 8, 2016, 1:51 p.m.
On Thu, 07 Jan 2016 12:00:38 -0800, Sean Farley wrote:
> Yuya Nishihara <yuya@tcha.org> writes:
> > On Tue, 05 Jan 2016 20:48:34 +0900, FUJIWARA Katsunori wrote:
> >> +            if util.safehasattr(module, objname):
> >
> > Nit: you can use getattr(module, objname, None).
> 
> Should we remove util.safehasattr now?

(CC-ing Augie)

Perhaps no.

hasattr() of Python 2.6 should no longer eat KeyboardInterrupt [1], but it
still ignores Exception [2], whereas our safehasattr() only catches
AttributeError [3].

 [1]: https://bugs.python.org/issue2196
 [2]: https://hg.python.org/cpython/file/v2.7.11/Python/bltinmodule.c#l906
 [3]: https://hg.python.org/cpython/file/v2.7.11/Python/bltinmodule.c#l849
Augie Fackler - Jan. 8, 2016, 3:11 p.m.
On Jan 8, 2016 8:53 AM, "Yuya Nishihara" <yuya@tcha.org> wrote:
>
> On Thu, 07 Jan 2016 12:00:38 -0800, Sean Farley wrote:
> > Yuya Nishihara <yuya@tcha.org> writes:
> > > On Tue, 05 Jan 2016 20:48:34 +0900, FUJIWARA Katsunori wrote:
> > >> +            if util.safehasattr(module, objname):
> > >
> > > Nit: you can use getattr(module, objname, None).
> >
> > Should we remove util.safehasattr now?
>
> (CC-ing Augie)
>
> Perhaps no.
>
> hasattr() of Python 2.6 should no longer eat KeyboardInterrupt [1], but it
> still ignores Exception [2], whereas our safehasattr() only catches
> AttributeError [3].

Yep. hasattr() is problematic until Python 3.2. KeyboardInterrupt was added
as a one-off ignore in 2.6, but they felt it too dangerous to actually fix
things all the way.

>
>  [1]: https://bugs.python.org/issue2196
>  [2]: https://hg.python.org/cpython/file/v2.7.11/Python/bltinmodule.c#l906
>  [3]: https://hg.python.org/cpython/file/v2.7.11/Python/bltinmodule.c#l849

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -749,6 +749,24 @@  def _checkshellalias(lui, ui, args, prec
     restorecommands()
 
 _loaded = set()
+
+def _loadcmdtable(ui, name, cmdtable):
+    overrides = [cmd for cmd in cmdtable if cmd in commands.table]
+    if overrides:
+        ui.warn(_("extension '%s' overrides commands: %s\n")
+                % (name, " ".join(overrides)))
+    commands.table.update(cmdtable)
+
+# list of (objname, loadermod, loadername) tuple:
+# - objname is the name of an object in extension module, from which
+#   extra information is loaded
+# - loadermod is the module where loader is placed
+# - loadername is the name of the function, which takes (ui, objname,
+#   obj) arguments
+extraloaders = [
+    ('cmdtable', None, None), # this is just a place holder for 'cmdtable'
+]
+
 def _dispatch(req):
     args = req.args
     ui = req.ui
@@ -777,13 +795,14 @@  def _dispatch(req):
 
     # (uisetup and extsetup are handled in extensions.loadall)
 
+    loadercache = {'cmdtable': _loadcmdtable}
     for name, module in exts:
-        cmdtable = getattr(module, 'cmdtable', {})
-        overrides = [cmd for cmd in cmdtable if cmd in commands.table]
-        if overrides:
-            ui.warn(_("extension '%s' overrides commands: %s\n")
-                    % (name, " ".join(overrides)))
-        commands.table.update(cmdtable)
+        for objname, loadermod, loadername in extraloaders:
+            if util.safehasattr(module, objname):
+                if objname not in loadercache:
+                    loadercache[objname] = getattr(loadermod, loadername)
+                loader = loadercache[objname]
+                loader(ui, name, getattr(module, objname))
         _loaded.add(name)
 
     # (reposetup is handled in hg.repository)