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

[Yuya Nishihara]

On Fri, 21 Dec 2018 21:17:39 -0500, Matt Harbison wrote:
> On Fri, 14 Dec 2018 06:54:11 -0500, Yuya Nishihara <yuya at 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.