[PATCH 2 of 4] dispatch: add an additional extensibility point during dispatch

[Gregory Szorc]

On Wed, Feb 4, 2015 at 3:52 PM, Matt Mackall <mpm at selenic.com> wrote:

> On Wed, 2015-02-04 at 15:42 -0800, Gregory Szorc wrote:
> > On Wed, Feb 4, 2015 at 2:43 PM, Matt Mackall <mpm at selenic.com> wrote:
> >
> > > On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc at gmail.com>
> > > > # Date 1423087102 28800
> > > > #      Wed Feb 04 13:58:22 2015 -0800
> > > > # Node ID f6866444b15b1bb1c5e24ea31660e1f8deb78a83
> > > > # Parent  c1b76e9330b1f39342899d8d53c3ddab388c3cb5
> > > > dispatch: add an additional extensibility point during dispatch
> > > >
> > > > An upcoming patch will introduce an ordering dependency between the
> > > > color and pager extensions. Specifically, one's uisetup function must
> > > > come before the other's.
> > > >
> > > > There is no easy way to accomplish guaranteed ordering of extension
> > > > functions. The Mercurial project has survived many years without an
> > > > explicit dependency system between extensions. Instead of inventing
> > > > one to solve this problem, we instead create a new, separate
> monkeypatch
> > > > point for the color extension to use to ensure its code runs after
> > > > pager's. It is ugly. But it is simple.
> > >
> > > This is pretty simple too:
> > >
> > > diff -r 7f375d2de945 mercurial/extensions.py
> > > --- a/mercurial/extensions.py   Mon Jan 26 14:30:12 2015 -0500
> > > +++ b/mercurial/extensions.py   Wed Feb 04 16:42:12 2015 -0600
> > > @@ -10,6 +10,7 @@
> > >  from i18n import _, gettext
> > >
> > >  _extensions = {}
> > > +_aftercallbacks = {}
> > >  _order = []
> > >  _ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg',
> 'inotify']
> > >
> > > @@ -87,6 +88,8 @@
> > >              mod = importh(name)
> > >      _extensions[shortname] = mod
> > >      _order.append(shortname)
> > > +    if shortname in _aftercallbacks:
> > > +        _aftercallbacks[shortname]()
> > >      return mod
> > >
> > >  def loadall(ui):
> > > @@ -123,6 +126,15 @@
> > >                      raise
> > >                  extsetup() # old extsetup with no ui argument
> > >
> > > +def aftersetup(extension, callback):
> > > +    """Run the specified function after the named extension is set
> up, or
> > > +    immediately"""
> > > +
> > > +    if extension in _extensions:
> > > +        callback()
> > > +    else:
> > > +        _aftercallbacks[extension] = callback
> > > +
> > >  def wrapcommand(table, command, wrapper):
> > >      '''Wrap the command named `command' in table
> > >
> > > Is it sufficient for your purposes? There are a few other places that
> > > want this sort of thing.
> >
> >
> > I think I can make this work. Although, the problem is still that pager
> and
> > color both do their magic at command dispatch time, not uisetup() time.
> >
> > But there is one part that's a bit wonky. If color does an
> > aftersetup('pager') and pager is never loaded, the callback never fires.
>
> Yeah, I figured you'd just put the pager-specific logic in there?
>

Well, the code path is /slightly/ more complicated because we have to
resolve "auto". I kinda like how part 4 is just 2 additional lines to color.


> > I started writing a patch that mucked about extensions._order. You could
> > even have extensions declare "after" or "before" dependencies via module
> > variables. I thought this was too much scope bloat.
>
> I think the "usual" usage is going to be something like extension X
> wants to call wrapcommand on on Y.Z... iff Y is loaded. The after
> callback is well-suited to that.
>

Just to make sure I am grokking you, you are advocating wrapping the
wrapper function from another extension. e.g.

def afterpagerdispatch(orig, innerorig, ui, *args):
   if getattr(ui, 'pageractive'):
       ...

   return orig(innerorig, ui, *args)

def aftersetup():
    pager = extensions.find('pager')
    extensions.wrapcommand(pager, '_wrapcommand', afterpagerdispatch)

Won't the addition of the "orig" function to the arguments list for every
nested wrapping lead to problems (you don't know what level your wrapper is
installed at so argument positions won't be constant)?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150204/73b65911/attachment.html>