Patchwork [1,of,6] extensions: import the exthelper class from evolve ff799015d62e

login
register
mail settings
Submitter Matt Harbison
Date Dec. 1, 2018, 4:10 a.m.
Message ID <d5a04a8016a270dce028.1543637455@Envy>
Download mbox | patch
Permalink /patch/36888/
State New
Headers show

Comments

Matt Harbison - Dec. 1, 2018, 4:10 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1543121473 18000
#      Sat Nov 24 23:51:13 2018 -0500
# Node ID d5a04a8016a270dce028bddb2483509e0429f113
# Parent  33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
extensions: import the exthelper class from evolve ff799015d62e

This should help make extensions that wrap a lot of stuff more comprehendible.

It was copied unmodified, except:
  - fix up the imports
  - rename final_xxxsetup() -> finalxxxsetup() to appease checkcode
  - avoid a [] default arg to wrapcommand()
  - adjust the module name used to load the templates and revsets

It would be nice to figure out the extension that instantiates the class
automatically to handle the last item.  But since the load functions don't
actually do anything with this string, I think the TODO is good enough for now.

The helper class currently lacks uipopulate() and fileset support.

I'm not sure what licensing info to put in place, since it wasn't available on
the original copy.
Yuya Nishihara - Dec. 2, 2018, 11:37 a.m.
On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1543121473 18000
> #      Sat Nov 24 23:51:13 2018 -0500
> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
> # Parent  33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
> extensions: import the exthelper class from evolve ff799015d62e

> I'm not sure what licensing info to put in place, since it wasn't available on
> the original copy.

Boris?

> +    def configitem(self, section, config, default=configitems.dynamicdefault):
> +        """Register a config item.
> +        """
> +        self._configitem(section, config, default=default)

What's the point of reinventing the registrar functions?

I understand it will be useful to provide decorators for various setup
hooks, but we have registrar API for table registrations. And less coupled
abstraction is generally better.
Matt Harbison - Dec. 2, 2018, 2:31 p.m.
>> On Dec 2, 2018, at 6:37 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> 
>> On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1543121473 18000
>> #      Sat Nov 24 23:51:13 2018 -0500
>> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
>> # Parent  33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
>> extensions: import the exthelper class from evolve ff799015d62e
> 
>> I'm not sure what licensing info to put in place, since it wasn't available on
>> the original copy.
> 
> Boris?
> 
>> +    def configitem(self, section, config, default=configitems.dynamicdefault):
>> +        """Register a config item.
>> +        """
>> +        self._configitem(section, config, default=default)
> 
> What's the point of reinventing the registrar functions?
> 
> I understand it will be useful to provide decorators for various setup
> hooks, but we have registrar API for table registrations. And less coupled
> abstraction is generally better.

I wondered about that too. OTOH, then it’s consistent with everything else and you don’t need to add this boilerplate (which isn’t hard, but I never remember exactly what it is):

configtable = {}
configitem = registrar.configitem(configtable)

(Unless you also meant fileset/revset/template registrations shouldn’t be duplicated too.)

I wasn’t thrilled with the class name and first tried putting this in the extensions module, but I think there was a cycle. Maybe that will go away if we boil this down to just the setup functions.
Matt Harbison - Dec. 3, 2018, 1:59 a.m.
On Sun, 02 Dec 2018 06:37:12 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1543121473 18000
>> #      Sat Nov 24 23:51:13 2018 -0500
>> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
>> # Parent  33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
>> extensions: import the exthelper class from evolve ff799015d62e
>
>> I'm not sure what licensing info to put in place, since it wasn't  
>> available on
>> the original copy.
>
> Boris?
>
>> +    def configitem(self, section, config,  
>> default=configitems.dynamicdefault):
>> +        """Register a config item.
>> +        """
>> +        self._configitem(section, config, default=default)
>
> What's the point of reinventing the registrar functions?
>
> I understand it will be useful to provide decorators for various setup
> hooks, but we have registrar API for table registrations. And less  
> coupled
> abstraction is generally better.

On second thought, it looks like there's code in there to help wrap  
commands added by other extensions.  See self._extcommandwrappers.  I  
guess we don't need to reinvent configitem/fileset/revset/template, but  
maybe that's enough reason to keep command registration?
Boris Feld - Dec. 3, 2018, 8:37 a.m.
On 12/2/18 12:37 PM, Yuya Nishihara wrote:
> On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1543121473 18000
>> # Sat Nov 24 23:51:13 2018 -0500
>> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
>> # Parent 33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
>> extensions: import the exthelper class from evolve ff799015d62e

>> It was copied unmodified, except:
>> - fix up the imports
>> - rename final_xxxsetup() -> finalxxxsetup() to appease checkcode
>> - avoid a [] default arg to wrapcommand()
>> - adjust the module name used to load the templates and revsets

It would be nice if we kept the same API for the evolve and core, this
would help evolve to smoothly transition to the core one eventually.

Should we keep the `final_` form in core or should we move to the more
compact one for evolve?

The module name for templates and revsets could be a mandatory argument
to the exthelper object (at least the top level one).

eh = exthelper('lfs')

(note: We pushed a couple of the API improvement to evolve.)


>> I'm not sure what licensing info to put in place, since it wasn't
>> available on
>> the original copy.
>
> Boris?

 Something similar to the extension header would do. We pushed a
changeset to clarify copyright and license of the exthelper.py module to
evolve. You can reuse that.

>> + def configitem(self, section, config,
>> default=configitems.dynamicdefault):
>> + """Register a config item.
>> + """
>> + self._configitem(section, config, default=default)
>
> What's the point of reinventing the registrar functions?

Strictly speaking, the registrar is reinventing exthelper ;-) exthelper
has been around for twice as long :-)

> I understand it will be useful to provide decorators for various setup
> hooks, but we have registrar API for table registrations. And less coupled
> abstraction is generally better.

The exthelper method to register items stayed around after registrar
introduction because of one specific feature: exthelper merging.

A key feature of registrar is to be able to combine submodule instance
with top level one. You create an instance in the sub module and
decorate everything that needs to be decorated. Then in the higher level
module, you can simply merge the submodule exthelper instance with the
local one, everything necessary declaration will be propagated.

The registrar API requires the xxxtable to be explicitly declared and
passed to the registrar function. This is a significant limitation.
Yuya Nishihara - Dec. 3, 2018, 11:20 a.m.
On Sun, 02 Dec 2018 20:59:42 -0500, Matt Harbison wrote:
> On Sun, 02 Dec 2018 06:37:12 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1543121473 18000
> >> #      Sat Nov 24 23:51:13 2018 -0500
> >> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
> >> # Parent  33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
> >> extensions: import the exthelper class from evolve ff799015d62e
> >
> >> I'm not sure what licensing info to put in place, since it wasn't  
> >> available on
> >> the original copy.
> >
> > Boris?
> >
> >> +    def configitem(self, section, config,  
> >> default=configitems.dynamicdefault):
> >> +        """Register a config item.
> >> +        """
> >> +        self._configitem(section, config, default=default)
> >
> > What's the point of reinventing the registrar functions?
> >
> > I understand it will be useful to provide decorators for various setup
> > hooks, but we have registrar API for table registrations. And less  
> > coupled
> > abstraction is generally better.
> 
> On second thought, it looks like there's code in there to help wrap  
> commands added by other extensions.  See self._extcommandwrappers.  I  
> guess we don't need to reinvent configitem/fileset/revset/template, but  
> maybe that's enough reason to keep command registration?

Maybe no. Though I won't strongly disagree with that, I'm not interested
in duplicating the functionality unless the new one is superior.
Yuya Nishihara - Dec. 3, 2018, 11:49 a.m.
On Mon, 3 Dec 2018 09:37:07 +0100, Boris FELD wrote:
> On 12/2/18 12:37 PM, Yuya Nishihara wrote:
> > On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1543121473 18000
> >> # Sat Nov 24 23:51:13 2018 -0500
> >> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
> >> # Parent 33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
> >> extensions: import the exthelper class from evolve ff799015d62e
> 
> >> It was copied unmodified, except:
> >> - fix up the imports
> >> - rename final_xxxsetup() -> finalxxxsetup() to appease checkcode
> >> - avoid a [] default arg to wrapcommand()
> >> - adjust the module name used to load the templates and revsets
> 
> It would be nice if we kept the same API for the evolve and core, this
> would help evolve to smoothly transition to the core one eventually.

Well, it can also be said that the evolve can be ported to the core
extension API once it dropped compatibility to old hg versions.

> > I understand it will be useful to provide decorators for various setup
> > hooks, but we have registrar API for table registrations. And less coupled
> > abstraction is generally better.
> 
> The exthelper method to register items stayed around after registrar
> introduction because of one specific feature: exthelper merging.
> 
> A key feature of registrar is to be able to combine submodule instance
> with top level one. You create an instance in the sub module and
> decorate everything that needs to be decorated. Then in the higher level
> module, you can simply merge the submodule exthelper instance with the
> local one, everything necessary declaration will be propagated.

It might be nice feature if there were lots of sub-modules providing
uisetup() for example, and uisetup() is that sort of function. I agree.
However, revsets/templates/filesets are likely to be defined in one or two
modules, which can be manageable with the current registrar API even if
the extension is "evolve" scale.

I'm not saying that the exthelper is better or worse than the current
registrar API, but IMHO the current API is good enough.

> The registrar API requires the xxxtable to be explicitly declared and
> passed to the registrar function. This is a significant limitation.

That's why @revsetpredicate() omits revsetpredicatetable.
Matt Harbison - Dec. 5, 2018, 5:08 a.m.
On Mon, 03 Dec 2018 03:37:07 -0500, Boris FELD <boris.feld@octobus.net>  
wrote:

> On 12/2/18 12:37 PM, Yuya Nishihara wrote:
>> On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1543121473 18000
>>> # Sat Nov 24 23:51:13 2018 -0500
>>> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
>>> # Parent 33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
>>> extensions: import the exthelper class from evolve ff799015d62e
>
>>> It was copied unmodified, except:
>>> - fix up the imports
>>> - rename final_xxxsetup() -> finalxxxsetup() to appease checkcode
>>> - avoid a [] default arg to wrapcommand()
>>> - adjust the module name used to load the templates and revsets
>
> It would be nice if we kept the same API for the evolve and core, this
> would help evolve to smoothly transition to the core one eventually.
>
> Should we keep the `final_` form in core or should we move to the more
> compact one for evolve?

I'd like to, but checkcode disagreed.  I know there's been talk about  
allowing '_' for readability; I'm not sure what the status of that is.   
Not sure if anyone else wants to chime in about this.

> The module name for templates and revsets could be a mandatory argument
> to the exthelper object (at least the top level one).
>
> eh = exthelper('lfs')

I thought about that, and I guess we can do that.  I was concerned that  
it's error prone, and I'm not sure how to make it mandatory for only the  
top level instance.

> (note: We pushed a couple of the API improvement to evolve.)

I'm not sure what to do here.  Yuya objected to the fileset one in these.   
I don't have a problem omitting this, the template and revset support, but  
I'm not sure what sort of problems that will cause in evolve.  I still  
think the command wrapping is worth keeping, due to the helper code to  
wrap other extension's commands.  Wrapping extension commands properly  
seems pretty rare (so few examples) and a bit obscure, so it seems better  
to not force the extension developer to recognize the problem and roll  
their own solution.

I'd like to get agreement before sending off the next iteration.

Does the name seem reasonable with this wider audience?  I wanted to put  
it inside the extensions module, but it looks like there's a cycle around  
commands.py.
Yuya Nishihara - Dec. 5, 2018, 1:33 p.m.
On Wed, 05 Dec 2018 00:08:24 -0500, Matt Harbison wrote:
> Does the name seem reasonable with this wider audience?  I wanted to put  
> it inside the extensions module, but it looks like there's a cycle around  
> commands.py.

I'm okay with the name exthelper as it is a utility-level function built
on top of core extension API. In order to resolve cycle, commands.table
could be passed in to the exthelper as an argument.
Boris Feld - Dec. 7, 2018, 2:42 p.m.
On 05/12/2018 14:33, Yuya Nishihara wrote:
> On Wed, 05 Dec 2018 00:08:24 -0500, Matt Harbison wrote:
>> Does the name seem reasonable with this wider audience?  I wanted to put  
>> it inside the extensions module, but it looks like there's a cycle around  
>> commands.py.
> I'm okay with the name exthelper as it is a utility-level function built
> on top of core extension API. In order to resolve cycle, commands.table
> could be passed in to the exthelper as an argument.

We might have to duplicate this logic for other kinds of object :-/ That
makes things less practical. Could we use something else? Maybe local
importing?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - Dec. 7, 2018, 2:44 p.m.
On 05/12/2018 06:08, Matt Harbison wrote:
> On Mon, 03 Dec 2018 03:37:07 -0500, Boris FELD
> <boris.feld@octobus.net> wrote:
>
>> On 12/2/18 12:37 PM, Yuya Nishihara wrote:
>>> On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1543121473 18000
>>>> # Sat Nov 24 23:51:13 2018 -0500
>>>> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
>>>> # Parent 33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
>>>> extensions: import the exthelper class from evolve ff799015d62e
>>
>>>> It was copied unmodified, except:
>>>> - fix up the imports
>>>> - rename final_xxxsetup() -> finalxxxsetup() to appease checkcode
>>>> - avoid a [] default arg to wrapcommand()
>>>> - adjust the module name used to load the templates and revsets
>>
>> It would be nice if we kept the same API for the evolve and core, this
>> would help evolve to smoothly transition to the core one eventually.
>>
>> Should we keep the `final_` form in core or should we move to the more
>> compact one for evolve?
>
> I'd like to, but checkcode disagreed.  I know there's been talk about
> allowing '_' for readability; I'm not sure what the status of that
> is.  Not sure if anyone else wants to chime in about this.
>
>> The module name for templates and revsets could be a mandatory argument
>> to the exthelper object (at least the top level one).
>>
>> eh = exthelper('lfs')
>
> I thought about that, and I guess we can do that.  I was concerned
> that it's error prone, and I'm not sure how to make it mandatory for
> only the top level instance.

What about this:

- Extension name is either declared or None.
- a.merge(b) will complain if extension name is different (unless b's
one is None)
- the function that needs a name will crash if the extension name is
None (so, if we failed to declare it at the root level)
>
>> (note: We pushed a couple of the API improvement to evolve.)
>
> I'm not sure what to do here.  Yuya objected to the fileset one in
> these.  I don't have a problem omitting this, the template and revset
> support, but I'm not sure what sort of problems that will cause in
> evolve.  I still think the command wrapping is worth keeping, due to
> the helper code to wrap other extension's commands.  Wrapping
> extension commands properly seems pretty rare (so few examples) and a
> bit obscure, so it seems better to not force the extension developer
> to recognize the problem and roll their own solution.
>
> I'd like to get agreement before sending off the next iteration.

Would it be possible to have the registrar command able to take an
exthelper object as parameter (as an alternative to their xxxtable).

This way we don't need to duplicate the registrar's function API but we
keep the advantage of the exthelper.
>
> Does the name seem reasonable with this wider audience?  I wanted to
> put it inside the extensions module, but it looks like there's a cycle
> around commands.py.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - Dec. 7, 2018, 5:56 p.m.
On 03/12/2018 12:49, Yuya Nishihara wrote:
> On Mon, 3 Dec 2018 09:37:07 +0100, Boris FELD wrote:
>> On 12/2/18 12:37 PM, Yuya Nishihara wrote:
>>> On Fri, 30 Nov 2018 23:10:55 -0500, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1543121473 18000
>>>> # Sat Nov 24 23:51:13 2018 -0500
>>>> # Node ID d5a04a8016a270dce028bddb2483509e0429f113
>>>> # Parent 33d30fb1e4ae52a283ef487ccf5dfbe59b8a5a68
>>>> extensions: import the exthelper class from evolve ff799015d62e
>>>> It was copied unmodified, except:
>>>> - fix up the imports
>>>> - rename final_xxxsetup() -> finalxxxsetup() to appease checkcode
>>>> - avoid a [] default arg to wrapcommand()
>>>> - adjust the module name used to load the templates and revsets
>> It would be nice if we kept the same API for the evolve and core, this
>> would help evolve to smoothly transition to the core one eventually.
> Well, it can also be said that the evolve can be ported to the core
> extension API once it dropped compatibility to old hg versions.

I don't have a strong opinion about either form (with or without _).

I would prefer if we update Evolve first with the new API then import
the resulting exthelper version in core. This way the imported code is
as close as possible to Evolve's exthelper version and would ease
migrating Evolve to use the core version and "backport" updates made on
Core exthelper to Evolve.
>
>>> I understand it will be useful to provide decorators for various setup
>>> hooks, but we have registrar API for table registrations. And less coupled
>>> abstraction is generally better.
>> The exthelper method to register items stayed around after registrar
>> introduction because of one specific feature: exthelper merging.
>>
>> A key feature of registrar is to be able to combine submodule instance
>> with top level one. You create an instance in the sub module and
>> decorate everything that needs to be decorated. Then in the higher level
>> module, you can simply merge the submodule exthelper instance with the
>> local one, everything necessary declaration will be propagated.
> It might be nice feature if there were lots of sub-modules providing
> uisetup() for example, and uisetup() is that sort of function. I agree.
> However, revsets/templates/filesets are likely to be defined in one or two
> modules, which can be manageable with the current registrar API even if
> the extension is "evolve" scale.
>
> I'm not saying that the exthelper is better or worse than the current
> registrar API, but IMHO the current API is good enough.
>
>> The registrar API requires the xxxtable to be explicitly declared and
>> passed to the registrar function. This is a significant limitation.
> That's why @revsetpredicate() omits revsetpredicatetable.
In practice this flexibility is nice. What about having the registrar
function accept (and understand an exthelper object?) (cf my other reply
to Matt Harbisson)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 9, 2018, 5:29 a.m.
On Fri, 7 Dec 2018 15:44:44 +0100, Boris FELD wrote:
> > I'm not sure what to do here.  Yuya objected to the fileset one in
> > these.  I don't have a problem omitting this, the template and revset
> > support, but I'm not sure what sort of problems that will cause in
> > evolve.  I still think the command wrapping is worth keeping, due to
> > the helper code to wrap other extension's commands.  Wrapping
> > extension commands properly seems pretty rare (so few examples) and a
> > bit obscure, so it seems better to not force the extension developer
> > to recognize the problem and roll their own solution.
> >
> > I'd like to get agreement before sending off the next iteration.
> 
> Would it be possible to have the registrar command able to take an
> exthelper object as parameter (as an alternative to their xxxtable).
> 
> This way we don't need to duplicate the registrar's function API but we
> keep the advantage of the exthelper.

Can you elaborate?

IIUC, the control flows of the registrar and the exthelper are different.
With registrar, the extension loader loads functions from a static table,
but with exthelper, functions are installed by a setup function. That makes
me feel they are inherently incompatible.

An alternative idea is to provide a thin registrar wrapper backed by an
exthelper table.

  class exthelper:
      def __init__:
          self.xxx = registrar.xxx(self._xxxtable)

  # in top-level extension module
  xxx = eh.xxx

  @eh.xxx(...)
  def ...
Boris Feld - Dec. 14, 2018, 10:18 a.m.
On 09/12/2018 05:29, Yuya Nishihara wrote:
> On Fri, 7 Dec 2018 15:44:44 +0100, Boris FELD wrote:
>>> I'm not sure what to do here.  Yuya objected to the fileset one in
>>> these.  I don't have a problem omitting this, the template and revset
>>> support, but I'm not sure what sort of problems that will cause in
>>> evolve.  I still think the command wrapping is worth keeping, due to
>>> the helper code to wrap other extension's commands.  Wrapping
>>> extension commands properly seems pretty rare (so few examples) and a
>>> bit obscure, so it seems better to not force the extension developer
>>> to recognize the problem and roll their own solution.
>>>
>>> I'd like to get agreement before sending off the next iteration.
>> Would it be possible to have the registrar command able to take an
>> exthelper object as parameter (as an alternative to their xxxtable).
>>
>> This way we don't need to duplicate the registrar's function API but we
>> keep the advantage of the exthelper.
> Can you elaborate?
>
> IIUC, the control flows of the registrar and the exthelper are different.
> With registrar, the extension loader loads functions from a static table,
> but with exthelper, functions are installed by a setup function. That makes
> me feel they are inherently incompatible.

The exthelper uses setup function because it predates the registrar. We
can change the underlying implementation. The core of the exthelper
class goals are simple:

1) provide a decorator based solution for all declarations
2) allow to recursively combine instance from extension's submodules

Registrar covers the decorator aspect (for everything with a registrar),
but not the recursive one.

Here is what I have in mind, the exthelper object would be a valid
argument for registrar. The following code would be valid.

  eh = exthelper('myextension')
  command = registar.command(eh)
  commandtable = eh.commandtable

  @command(…)

This requires the registrar to recognize that its argument is an
exthelper instance and look for the right table there.

Such API could be easily improved by having the extensions logic
directly look for table inside the exthelper. Let us rename it to
"register" as it would better fit its new usage

  from mercurial import extension, registrar
  from . import subitem

  register = extensions.register('myextension')
  register.merge(subitem.register)

  command = registrar.command(register)

  @command(…)

>
> An alternative idea is to provide a thin registrar wrapper backed by an
> exthelper table.
>
>   class exthelper:
>       def __init__:
>           self.xxx = registrar.xxx(self._xxxtable)
>
>   # in top-level extension module
>   xxx = eh.xxx
>
>   @eh.xxx(...)
>   def ...
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 14, 2018, 11:54 a.m.
On Fri, 14 Dec 2018 10:18:42 +0000, Boris FELD wrote:
> On 09/12/2018 05:29, Yuya Nishihara wrote:
> > On Fri, 7 Dec 2018 15:44:44 +0100, Boris FELD wrote:
> >>> I'm not sure what to do here.  Yuya objected to the fileset one in
> >>> these.  I don't have a problem omitting this, the template and revset
> >>> support, but I'm not sure what sort of problems that will cause in
> >>> evolve.  I still think the command wrapping is worth keeping, due to
> >>> the helper code to wrap other extension's commands.  Wrapping
> >>> extension commands properly seems pretty rare (so few examples) and a
> >>> bit obscure, so it seems better to not force the extension developer
> >>> to recognize the problem and roll their own solution.
> >>>
> >>> I'd like to get agreement before sending off the next iteration.
> >> Would it be possible to have the registrar command able to take an
> >> exthelper object as parameter (as an alternative to their xxxtable).
> >>
> >> This way we don't need to duplicate the registrar's function API but we
> >> keep the advantage of the exthelper.
> > Can you elaborate?
> >
> > IIUC, the control flows of the registrar and the exthelper are different.
> > With registrar, the extension loader loads functions from a static table,
> > but with exthelper, functions are installed by a setup function. That makes
> > me feel they are inherently incompatible.
> 
> The exthelper uses setup function because it predates the registrar. We
> can change the underlying implementation. The core of the exthelper
> class goals are simple:
> 
> 1) provide a decorator based solution for all declarations
> 2) allow to recursively combine instance from extension's submodules
> 
> Registrar covers the decorator aspect (for everything with a registrar),
> but not the recursive one.
> 
> Here is what I have in mind, the exthelper object would be a valid
> argument for registrar. The following code would be valid.
> 
>   eh = exthelper('myextension')
>   command = registar.command(eh)
>   commandtable = eh.commandtable
> 
>   @command(…)

I don't think the registrar functions have to be overloaded for the exthelper.
Instead, eh.commandtable can be passed in:

  command = registrar.command(eh.commandtable)

And if we want to make the eh.commandtable private, we can move the decorator
to the exthelper:

> >   class exthelper:
> >       def __init__:
> >           self.xxx = registrar.xxx(self._xxxtable)
> >
> >   # in top-level extension module
> >   xxx = eh.xxx
> >
> >   @eh.xxx(...)
> >   def ...

This doesn't increase the complexity. In order to make registrar functions
accept an exthelper instance, the exthelper would have to prepare tables
for all the known registrar functions. So the exthelper has the knowledge
about the registrar anyway. No need to make both registrar and exthelper
know each other.
Boris Feld - Dec. 14, 2018, 9:01 p.m.
On 14/12/2018 11:54, Yuya Nishihara wrote:
> On Fri, 14 Dec 2018 10:18:42 +0000, Boris FELD wrote:
>> On 09/12/2018 05:29, Yuya Nishihara wrote:
>>> On Fri, 7 Dec 2018 15:44:44 +0100, Boris FELD wrote:
>>>>> I'm not sure what to do here.  Yuya objected to the fileset one in
>>>>> these.  I don't have a problem omitting this, the template and revset
>>>>> support, but I'm not sure what sort of problems that will cause in
>>>>> evolve.  I still think the command wrapping is worth keeping, due to
>>>>> the helper code to wrap other extension's commands.  Wrapping
>>>>> extension commands properly seems pretty rare (so few examples) and a
>>>>> bit obscure, so it seems better to not force the extension developer
>>>>> to recognize the problem and roll their own solution.
>>>>>
>>>>> I'd like to get agreement before sending off the next iteration.
>>>> Would it be possible to have the registrar command able to take an
>>>> exthelper object as parameter (as an alternative to their xxxtable).
>>>>
>>>> This way we don't need to duplicate the registrar's function API but we
>>>> keep the advantage of the exthelper.
>>> Can you elaborate?
>>>
>>> IIUC, the control flows of the registrar and the exthelper are different.
>>> With registrar, the extension loader loads functions from a static table,
>>> but with exthelper, functions are installed by a setup function. That makes
>>> me feel they are inherently incompatible.
>> The exthelper uses setup function because it predates the registrar. We
>> can change the underlying implementation. The core of the exthelper
>> class goals are simple:
>>
>> 1) provide a decorator based solution for all declarations
>> 2) allow to recursively combine instance from extension's submodules
>>
>> Registrar covers the decorator aspect (for everything with a registrar),
>> but not the recursive one.
>>
>> Here is what I have in mind, the exthelper object would be a valid
>> argument for registrar. The following code would be valid.
>>
>>   eh = exthelper('myextension')
>>   command = registar.command(eh)
>>   commandtable = eh.commandtable
>>
>>   @command(…)
> I don't think the registrar functions have to be overloaded for the exthelper.
> Instead, eh.commandtable can be passed in:
>
>   command = registrar.command(eh.commandtable)
>
> And if we want to make the eh.commandtable private, we can move the decorator
> to the exthelper:
>
>>>   class exthelper:
>>>       def __init__:
>>>           self.xxx = registrar.xxx(self._xxxtable)
>>>
>>>   # in top-level extension module
>>>   xxx = eh.xxx
>>>
>>>   @eh.xxx(...)
>>>   def ...
> This doesn't increase the complexity. In order to make registrar functions
> accept an exthelper instance, the exthelper would have to prepare tables
> for all the known registrar functions. So the exthelper has the knowledge
> about the registrar anyway. No need to make both registrar and exthelper
> know each other.

As long as we can still simply combine exthelper, I'm fine. Your
proposal works for me.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Matt Harbison - Dec. 22, 2018, 2:17 a.m.
On Fri, 14 Dec 2018 06:54:11 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 14 Dec 2018 10:18:42 +0000, Boris FELD wrote:
>> On 09/12/2018 05:29, Yuya Nishihara wrote:
>> > On Fri, 7 Dec 2018 15:44:44 +0100, Boris FELD wrote:
>> >>> I'm not sure what to do here.  Yuya objected to the fileset one in
>> >>> these.  I don't have a problem omitting this, the template and  
>> revset
>> >>> support, but I'm not sure what sort of problems that will cause in
>> >>> evolve.  I still think the command wrapping is worth keeping, due to
>> >>> the helper code to wrap other extension's commands.  Wrapping
>> >>> extension commands properly seems pretty rare (so few examples) and  
>> a
>> >>> bit obscure, so it seems better to not force the extension developer
>> >>> to recognize the problem and roll their own solution.
>> >>>
>> >>> I'd like to get agreement before sending off the next iteration.
>> >> Would it be possible to have the registrar command able to take an
>> >> exthelper object as parameter (as an alternative to their xxxtable).
>> >>
>> >> This way we don't need to duplicate the registrar's function API but  
>> we
>> >> keep the advantage of the exthelper.
>> > Can you elaborate?
>> >
>> > IIUC, the control flows of the registrar and the exthelper are  
>> different.
>> > With registrar, the extension loader loads functions from a static  
>> table,
>> > but with exthelper, functions are installed by a setup function. That  
>> makes
>> > me feel they are inherently incompatible.
>>
>> The exthelper uses setup function because it predates the registrar. We
>> can change the underlying implementation. The core of the exthelper
>> class goals are simple:
>>
>> 1) provide a decorator based solution for all declarations
>> 2) allow to recursively combine instance from extension's submodules
>>
>> Registrar covers the decorator aspect (for everything with a registrar),
>> but not the recursive one.
>>
>> Here is what I have in mind, the exthelper object would be a valid
>> argument for registrar. The following code would be valid.
>>
>>   eh = exthelper('myextension')
>>   command = registar.command(eh)
>>   commandtable = eh.commandtable
>>
>>   @command(…)
>
> I don't think the registrar functions have to be overloaded for the  
> exthelper.
> Instead, eh.commandtable can be passed in:
>
>   command = registrar.command(eh.commandtable)
>
> And if we want to make the eh.commandtable private, we can move the  
> decorator
> to the exthelper:
>
>> >   class exthelper:
>> >       def __init__:
>> >           self.xxx = registrar.xxx(self._xxxtable)
>> >
>> >   # in top-level extension module
>> >   xxx = eh.xxx
>> >
>> >   @eh.xxx(...)
>> >   def ...

I think I'm confused.  It looks like commands and configitems are already  
registered this way.  The snippet of code that was quoted when you raised  
the initial question is configitem related.  So is the objection really  
just do the same thing for fileset/revset/template, and the rest is OK?
Yuya Nishihara - Dec. 22, 2018, 2:57 a.m.
On Fri, 21 Dec 2018 21:17:39 -0500, Matt Harbison wrote:
> On Fri, 14 Dec 2018 06:54:11 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Fri, 14 Dec 2018 10:18:42 +0000, Boris FELD wrote:
> >> On 09/12/2018 05:29, Yuya Nishihara wrote:
> >> > On Fri, 7 Dec 2018 15:44:44 +0100, Boris FELD wrote:
> >> >>> I'm not sure what to do here.  Yuya objected to the fileset one in
> >> >>> these.  I don't have a problem omitting this, the template and  
> >> revset
> >> >>> support, but I'm not sure what sort of problems that will cause in
> >> >>> evolve.  I still think the command wrapping is worth keeping, due to
> >> >>> the helper code to wrap other extension's commands.  Wrapping
> >> >>> extension commands properly seems pretty rare (so few examples) and  
> >> a
> >> >>> bit obscure, so it seems better to not force the extension developer
> >> >>> to recognize the problem and roll their own solution.
> >> >>>
> >> >>> I'd like to get agreement before sending off the next iteration.
> >> >> Would it be possible to have the registrar command able to take an
> >> >> exthelper object as parameter (as an alternative to their xxxtable).
> >> >>
> >> >> This way we don't need to duplicate the registrar's function API but  
> >> we
> >> >> keep the advantage of the exthelper.
> >> > Can you elaborate?
> >> >
> >> > IIUC, the control flows of the registrar and the exthelper are  
> >> different.
> >> > With registrar, the extension loader loads functions from a static  
> >> table,
> >> > but with exthelper, functions are installed by a setup function. That  
> >> makes
> >> > me feel they are inherently incompatible.
> >>
> >> The exthelper uses setup function because it predates the registrar. We
> >> can change the underlying implementation. The core of the exthelper
> >> class goals are simple:
> >>
> >> 1) provide a decorator based solution for all declarations
> >> 2) allow to recursively combine instance from extension's submodules
> >>
> >> Registrar covers the decorator aspect (for everything with a registrar),
> >> but not the recursive one.
> >>
> >> Here is what I have in mind, the exthelper object would be a valid
> >> argument for registrar. The following code would be valid.
> >>
> >>   eh = exthelper('myextension')
> >>   command = registar.command(eh)
> >>   commandtable = eh.commandtable
> >>
> >>   @command(…)
> >
> > I don't think the registrar functions have to be overloaded for the  
> > exthelper.
> > Instead, eh.commandtable can be passed in:
> >
> >   command = registrar.command(eh.commandtable)
> >
> > And if we want to make the eh.commandtable private, we can move the  
> > decorator
> > to the exthelper:
> >
> >> >   class exthelper:
> >> >       def __init__:
> >> >           self.xxx = registrar.xxx(self._xxxtable)
> >> >
> >> >   # in top-level extension module
> >> >   xxx = eh.xxx
> >> >
> >> >   @eh.xxx(...)
> >> >   def ...
> 
> I think I'm confused.  It looks like commands and configitems are already  
> registered this way.  The snippet of code that was quoted when you raised  
> the initial question is configitem related.

Looks like that, sorry. Perhaps I just scanned the entire series, and picked
the first one to comment on. My concern was duplicating the functionality of
the registrar in a way it would increase the maintenance burden.

FWIW, 'def configitem' can be just 'self.configtable = registrar..'.

> So is the objection really
> just do the same thing for fileset/revset/template, and the rest is OK?

Please remove the fileset, revset and templatekw handling from the exthelper.
Instead, they can be reimplemented in a similar way to self.command.

Patch

diff --git a/mercurial/exthelper.py b/mercurial/exthelper.py
new file mode 100644
--- /dev/null
+++ b/mercurial/exthelper.py
@@ -0,0 +1,297 @@ 
+#####################################################################
+### Extension helper                                              ###
+#####################################################################
+
+from __future__ import absolute_import
+
+from . import (
+    commands,
+    configitems,
+    extensions,
+    registrar,
+    revset,
+    templatekw,
+)
+
+class exthelper(object):
+    """Helper for modular extension setup
+
+    A single helper should be instantiated for each extension. Helper
+    methods are then used as decorators for various purpose.
+
+    All decorators return the original function and may be chained.
+    """
+
+    def __init__(self):
+        self._uicallables = []
+        self._extcallables = []
+        self._repocallables = []
+        self._revsetsymbols = []
+        self._templatekws = []
+        self._commandwrappers = []
+        self._extcommandwrappers = []
+        self._functionwrappers = []
+        self._duckpunchers = []
+        self.cmdtable = {}
+        self.command = registrar.command(self.cmdtable)
+        if '^init' in commands.table:
+            olddoregister = self.command._doregister
+
+            def _newdoregister(self, name, *args, **kwargs):
+                if kwargs.pop('helpbasic', False):
+                    name = '^' + name
+                return olddoregister(self, name, *args, **kwargs)
+            self.command._doregister = _newdoregister
+
+        self.configtable = {}
+        self._configitem = registrar.configitem(self.configtable)
+
+    def configitem(self, section, config, default=configitems.dynamicdefault):
+        """Register a config item.
+        """
+        self._configitem(section, config, default=default)
+
+    def merge(self, other):
+        self._uicallables.extend(other._uicallables)
+        self._extcallables.extend(other._extcallables)
+        self._repocallables.extend(other._repocallables)
+        self._revsetsymbols.extend(other._revsetsymbols)
+        self._templatekws.extend(other._templatekws)
+        self._commandwrappers.extend(other._commandwrappers)
+        self._extcommandwrappers.extend(other._extcommandwrappers)
+        self._functionwrappers.extend(other._functionwrappers)
+        self._duckpunchers.extend(other._duckpunchers)
+        self.cmdtable.update(other.cmdtable)
+        for section, items in other.configtable.iteritems():
+            if section in self.configtable:
+                self.configtable[section].update(items)
+            else:
+                self.configtable[section] = items
+
+    def finaluisetup(self, ui):
+        """Method to be used as the extension uisetup
+
+        The following operations belong here:
+
+        - Changes to ui.__class__ . The ui object that will be used to run the
+          command has not yet been created. Changes made here will affect ui
+          objects created after this, and in particular the ui that will be
+          passed to runcommand
+        - Command wraps (extensions.wrapcommand)
+        - Changes that need to be visible to other extensions: because
+          initialization occurs in phases (all extensions run uisetup, then all
+          run extsetup), a change made here will be visible to other extensions
+          during extsetup
+        - Monkeypatch or wrap function (extensions.wrapfunction) of dispatch
+          module members
+        - Setup of pre-* and post-* hooks
+        - pushkey setup
+        """
+        for cont, funcname, func in self._duckpunchers:
+            setattr(cont, funcname, func)
+        for command, wrapper, opts in self._commandwrappers:
+            entry = extensions.wrapcommand(commands.table, command, wrapper)
+            if opts:
+                for short, long, val, msg in opts:
+                    entry[1].append((short, long, val, msg))
+        for cont, funcname, wrapper in self._functionwrappers:
+            extensions.wrapfunction(cont, funcname, wrapper)
+        for c in self._uicallables:
+            c(ui)
+
+    def finalextsetup(self, ui):
+        """Method to be used as a the extension extsetup
+
+        The following operations belong here:
+
+        - Changes depending on the status of other extensions. (if
+          extensions.find('mq'))
+        - Add a global option to all commands
+        - Register revset functions
+        """
+        knownexts = {}
+
+        revsetpredicate = registrar.revsetpredicate()
+        for name, symbol in self._revsetsymbols:
+            revsetpredicate(name)(symbol)
+        # TODO: Figure out the calling extension name
+        revset.loadpredicate(ui, 'exthelper', revsetpredicate)
+
+        templatekeyword = registrar.templatekeyword()
+        for name, kw, requires in self._templatekws:
+            if requires is not None:
+                templatekeyword(name, requires=requires)(kw)
+            else:
+                templatekeyword(name)(kw)
+        # TODO: Figure out the calling extension name
+        templatekw.loadkeyword(ui, 'exthelper', templatekeyword)
+
+        for ext, command, wrapper, opts in self._extcommandwrappers:
+            if ext not in knownexts:
+                try:
+                    e = extensions.find(ext)
+                except KeyError:
+                    # Extension isn't enabled, so don't bother trying to wrap
+                    # it.
+                    continue
+                knownexts[ext] = e.cmdtable
+            entry = extensions.wrapcommand(knownexts[ext], command, wrapper)
+            if opts:
+                for short, long, val, msg in opts:
+                    entry[1].append((short, long, val, msg))
+
+        for c in self._extcallables:
+            c(ui)
+
+    def finalreposetup(self, ui, repo):
+        """Method to be used as the extension reposetup
+
+        The following operations belong here:
+
+        - All hooks but pre-* and post-*
+        - Modify configuration variables
+        - Changes to repo.__class__, repo.dirstate.__class__
+        """
+        for c in self._repocallables:
+            c(ui, repo)
+
+    def uisetup(self, call):
+        """Decorated function will be executed during uisetup
+
+        example::
+
+            @eh.uisetup
+            def setupbabar(ui):
+                print 'this is uisetup!'
+        """
+        self._uicallables.append(call)
+        return call
+
+    def extsetup(self, call):
+        """Decorated function will be executed during extsetup
+
+        example::
+
+            @eh.extsetup
+            def setupcelestine(ui):
+                print 'this is extsetup!'
+        """
+        self._extcallables.append(call)
+        return call
+
+    def reposetup(self, call):
+        """Decorated function will be executed during reposetup
+
+        example::
+
+            @eh.reposetup
+            def setupzephir(ui, repo):
+                print 'this is reposetup!'
+        """
+        self._repocallables.append(call)
+        return call
+
+    def revset(self, symbolname):
+        """Decorated function is a revset symbol
+
+        The name of the symbol must be given as the decorator argument.
+        The symbol is added during `extsetup`.
+
+        example::
+
+            @eh.revset('hidden')
+            def revsetbabar(repo, subset, x):
+                args = revset.getargs(x, 0, 0, 'babar accept no argument')
+                return [r for r in subset if 'babar' in repo[r].description()]
+        """
+        def dec(symbol):
+            self._revsetsymbols.append((symbolname, symbol))
+            return symbol
+        return dec
+
+    def templatekw(self, keywordname, requires=None):
+        """Decorated function is a template keyword
+
+        The name of the keyword must be given as the decorator argument.
+        The symbol is added during `extsetup`.
+
+        example::
+
+            @eh.templatekw('babar')
+            def kwbabar(ctx):
+                return 'babar'
+        """
+        def dec(keyword):
+            self._templatekws.append((keywordname, keyword, requires))
+            return keyword
+        return dec
+
+    def wrapcommand(self, command, extension=None, opts=None):
+        """Decorated function is a command wrapper
+
+        The name of the command must be given as the decorator argument.
+        The wrapping is installed during `uisetup`.
+
+        If the second option `extension` argument is provided, the wrapping
+        will be applied in the extension commandtable. This argument must be a
+        string that will be searched using `extension.find` if not found and
+        Abort error is raised. If the wrapping applies to an extension, it is
+        installed during `extsetup`.
+
+        example::
+
+            @eh.wrapcommand('summary')
+            def wrapsummary(orig, ui, repo, *args, **kwargs):
+                ui.note('Barry!')
+                return orig(ui, repo, *args, **kwargs)
+
+        The `opts` argument allows specifying additional arguments for the
+        command.
+
+        """
+        def dec(wrapper):
+            if opts is None:
+                opts = []
+            if extension is None:
+                self._commandwrappers.append((command, wrapper, opts))
+            else:
+                self._extcommandwrappers.append((extension, command, wrapper,
+                                                 opts))
+            return wrapper
+        return dec
+
+    def wrapfunction(self, container, funcname):
+        """Decorated function is a function wrapper
+
+        This function takes two arguments, the container and the name of the
+        function to wrap. The wrapping is performed during `uisetup`.
+        (there is no extension support)
+
+        example::
+
+            @eh.function(discovery, 'checkheads')
+            def wrapfunction(orig, *args, **kwargs):
+                ui.note('His head smashed in and his heart cut out')
+                return orig(*args, **kwargs)
+        """
+        def dec(wrapper):
+            self._functionwrappers.append((container, funcname, wrapper))
+            return wrapper
+        return dec
+
+    def addattr(self, container, funcname):
+        """Decorated function is to be added to the container
+
+        This function takes two arguments, the container and the name of the
+        function to wrap. The wrapping is performed during `uisetup`.
+
+        example::
+
+            @eh.function(context.changectx, 'babar')
+            def babar(ctx):
+                return 'babar' in ctx.description
+        """
+        def dec(func):
+            self._duckpunchers.append((container, funcname, func))
+            return func
+        return dec