Patchwork [1,of,6] util: add event handling mechanism

# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1408422497 25200
#      Mon Aug 18 21:28:17 2014 -0700
# Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
# Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
util: add event handling mechanism

The patch adds a generic event handling mechanism to Mercurial. From a
high level, you create a named event, register some functions with that
event, and when you fire that event the registered functions get called.

As will be demonstrated in subsequent patches, event handling can be
considered an internal hooks mechanism and will provide a better
alternative to wrapping or monkeypatching.

The intent of the events system is to give extensions more well-defined
points for code insertion. Currently, extension authors have a limited
set of hooks and a giant pile of functions to choose from. When hooks
won't satisfy your requirements, you need to dig through a pile of code
to find an appropriate function to intercept. Then you need to replace
it and hope you remembered to call the original function properly.
It's a dangerous, inexact science.

Events, by contrast, will provide a well-defined set of points for
extensions to inject code. These require a simple code grep to locate
(".events.register()") and are less prone to common mistakes, such as
calling the original function incorrectly.

Events have another advantage in that they can be instance specific.
Monkeypatching often results in changing symbols on modules or
class types as opposed to individual methods on individual object
instances. Oftentimes you only want to apply customization to a single
instance of an object if that object meets certain criteria. In the
current world, you often have to globally replace and filter out
invocations that aren't appropriate. This is very dangerous. Events
support modifying behavior at a per-instance level and are thus much
safer.

On 8/18/2014 10:30 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1408422497 25200
> #      Mon Aug 18 21:28:17 2014 -0700
> # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> util: add event handling mechanism

> diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-eventmanager.py
> @@ -0,0 +1,109 @@
> +from mercurial.util import event, eventmanager, safehasattr
> +
> +import unittest
> +import silenttestrunner
> +
> +class testeventmanager(unittest.TestCase):
> +    def test_eventsimple(self):

test-check-commit-hg.t doesn't like this commit because it is
introducing a function with underscores. But I just stole this naming
convention from other unittest-based .py tests. I'm not sure who is in
the wrong here.

Dropping the _ still runs the tests.

On Mon, Aug 18, 2014 at 10:53:21PM -0700, Gregory Szorc wrote:
> On 8/18/2014 10:30 PM, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1408422497 25200
> > #      Mon Aug 18 21:28:17 2014 -0700
> > # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
> > # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> > util: add event handling mechanism
>
> > diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py
> > new file mode 100644
> > --- /dev/null
> > +++ b/tests/test-eventmanager.py
> > @@ -0,0 +1,109 @@
> > +from mercurial.util import event, eventmanager, safehasattr
> > +
> > +import unittest
> > +import silenttestrunner
> > +
> > +class testeventmanager(unittest.TestCase):
> > +    def test_eventsimple(self):
>
> test-check-commit-hg.t doesn't like this commit because it is
> introducing a function with underscores. But I just stole this naming
> convention from other unittest-based .py tests. I'm not sure who is in
> the wrong here.
>
> Dropping the _ still runs the tests.

I'm going to claim that test-check-commit-hg.t is too picky here, and
that consistency with the other unittest-derived testcases is
worthwhile.

A fix doesn't seem immediately obvious, but I'm also happy to just
ignore it for now.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

On Mon, Aug 18, 2014 at 10:30:13PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1408422497 25200
> #      Mon Aug 18 21:28:17 2014 -0700
> # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> util: add event handling mechanism
>
> The patch adds a generic event handling mechanism to Mercurial. From a
> high level, you create a named event, register some functions with that
> event, and when you fire that event the registered functions get called.

Interesting. I think this is probably worth pursuing in some form, but
I suspect mpm will take some amount of convincing.

>
> As will be demonstrated in subsequent patches, event handling can be
> considered an internal hooks mechanism and will provide a better
> alternative to wrapping or monkeypatching.
>
> The intent of the events system is to give extensions more well-defined
> points for code insertion. Currently, extension authors have a limited
> set of hooks and a giant pile of functions to choose from. When hooks
> won't satisfy your requirements, you need to dig through a pile of code
> to find an appropriate function to intercept. Then you need to replace
> it and hope you remembered to call the original function properly.
> It's a dangerous, inexact science.
>
> Events, by contrast, will provide a well-defined set of points for
> extensions to inject code. These require a simple code grep to locate
> (".events.register()") and are less prone to common mistakes, such as
> calling the original function incorrectly.

Calling out some example use cases here might go a bit towards
pitching it (that is, right in the commit message). I suspect you have
some bugzilla or codereview reasons to want this, and I probably want
them for some reason as well.

>
> Events have another advantage in that they can be instance specific.
> Monkeypatching often results in changing symbols on modules or
> class types as opposed to individual methods on individual object
> instances. Oftentimes you only want to apply customization to a single
> instance of an object if that object meets certain criteria. In the
> current world, you often have to globally replace and filter out
> invocations that aren't appropriate. This is very dangerous. Events
> support modifying behavior at a per-instance level and are thus much
> safer.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -2039,8 +2039,94 @@ class hooks(object):
>          for source, hook in self._hooks:
>              results.append(hook(*args))
>          return results
>
> +class event(object):
> +    '''An event with its handlers.
> +
> +    An ``event`` is essentially a collection of functions that will be invoked
> +    when the event fires. ``event`` instances are typically created through
> +    ``eventmanager`` instances.
> +
> +    Handler functions can be registered against an instance via the ``+=``
> +    operator. They can be unregistered via the ``-=`` operator.

Cute, but not very obvious. Can you make it
.register/.(unregister|clear) instead of doing the operator overload?

> +
> +    Handler functions can be invoked by calling an ``event`` instance like
> +    it is a function.
> +
> +    Handlers are executed in the order they are registered.

Does that actually matter? Might not be worth specifying in the API
contract like this.

> +
> +    The return value of handler functions is ignored.
> +
> +    When events are created, they are "bound" to 0 or more values which will
> +    be passed to every handler function in addition to the values passed to
> +    that event.
> +
> +    e.g.
> +
> +    >>> def handler(x, y, z):
> +    ...     print '%s %s %s' % (x, y, z)
> +    >>> e = event('foo', 'bar')
> +    >>> e += handler
> +    >>> e('baz')
> +    foo bar baz
> +    '''
> +
> +    def __init__(self, *args):
> +        # Convert to list to facilitate + operator later.
> +        self._args = list(args)
> +        self._handlers = []
> +
> +    def __iadd__(self, fn):
> +        if fn not in self._handlers:
> +            self._handlers.append(fn)
> +        return self
> +
> +    def __isub__(self, fn):
> +        self._handlers.remove(fn)
> +        return self
> +
> +    def __len__(self):
> +        return len(self._handlers)
> +
> +    def __call__(self, *args, **kwargs):
> +        args = self._args + list(args)
> +        for fn in self._handlers:
> +            fn(*args, **kwargs)
> +
> +class eventmanager(object):
> +    '''A collection of events.
> +
> +    This class powers the internal events system. Instances of this class are
> +    typically attached to an object, but they can be standalone.
> +
> +    Events are registered by calling the ``register`` function. Afterwards,
> +    the created ``event`` instance will be available under a property of the
> +    registered name.
> +
> +    e.g.
> +
> +    >>> m = eventmanager()
> +    >>> m.register('oncreate')
> +    >>> def handler():
> +    ...     print 'hello'
> +    >>> m.oncreate += handler
> +    >>> m.oncreate()
> +    hello
> +    '''
> +    def register(self, name, *args):
> +        """Register an event.
> +
> +        Events have names and default positional arguments. If positional
> +        arguments are defined, these argument will be passed as the first
> +        arguments to every invocation of the event.
> +
> +        A common pattern is to pass the ``self`` handle on whatever object
> +        is holding this ``eventmanager`` instance.
> +        """
> +        if not safehasattr(self, name):
> +            setattr(self, name, event(*args))
> +
>  def debugstacktrace(msg='stacktrace', skip=0, f=sys.stderr, otherf=sys.stdout):
>      '''Writes a message to f (stderr) with a nicely formatted stacktrace.
>      Skips the 'skip' last entries. By default it will flush stdout first.
>      It can be used everywhere and do intentionally not require an ui object.
> diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-eventmanager.py
> @@ -0,0 +1,109 @@
> +from mercurial.util import event, eventmanager, safehasattr
> +
> +import unittest
> +import silenttestrunner
> +
> +class testeventmanager(unittest.TestCase):
> +    def test_eventsimple(self):
> +        e = event()
> +        self.assertEqual(len(e), 0)
> +
> +        calls = {'h1': 0, 'h2': 0}
> +
> +        def h1():
> +            calls['h1'] += 1
> +
> +        def h2():
> +            calls['h2'] += 1
> +
> +        e += h1
> +        self.assertEqual(len(e), 1)
> +        e += h2
> +        self.assertEqual(len(e), 2)
> +        e += h2
> +        self.assertEqual(len(e), 2)
> +
> +        e()
> +        self.assertEqual(calls, {'h1': 1, 'h2': 1})
> +        e()
> +        self.assertEqual(calls, {'h1': 2, 'h2': 2})
> +
> +        e -= h1
> +        e()
> +        self.assertEqual(calls, {'h1': 2, 'h2': 3})
> +
> +    def test_eventarguments(self):
> +        e = event()
> +
> +        calls = {'h1': [], 'h2': []}
> +
> +        def h1(foo, bar, baz=False):
> +            calls['h1'].append((foo, bar, baz))
> +
> +        def h2(foo, bar, baz=True):
> +            calls['h2'].append((foo, bar, baz))
> +
> +        e += h1
> +        e += h2
> +
> +        e(1, 2, 3)
> +        self.assertEqual(calls, {'h1': [(1, 2, 3)], 'h2': [(1, 2, 3)]})
> +        e(3, 4, baz=None)
> +        self.assertEqual(calls, {'h1': [(1, 2, 3), (3, 4, None)],
> +                                 'h2': [(1, 2, 3), (3, 4, None)]})
> +        e(5, 6)
> +        self.assertEqual(calls, {'h1': [(1, 2, 3), (3, 4, None), (5, 6, False)],
> +                                 'h2': [(1, 2, 3), (3, 4, None), (5, 6, True)]})
> +
> +    def test_defaultargs(self):
> +        expected = [1, True]
> +        def h(obj, foo, bar=True):
> +            self.assertEqual(o, obj)
> +            self.assertEqual(foo, expected[0])
> +            self.assertEqual(bar, expected[1])
> +
> +        o = object()
> +        e = event(o)
> +        e += h
> +
> +        e(1)
> +        expected = [2, False]
> +        e(2, bar=False)
> +
> +    def test_eventmanager(self):
> +        class eventobject(object):
> +            def __init__(self):
> +                self.events = eventmanager()
> +
> +        o = eventobject()
> +        self.assertFalse(safehasattr(o.events, 'e1'))
> +        try:
> +            o.events.e1()
> +        except AttributeError:
> +            pass
> +        o.events.register('e1')
> +        self.assertTrue(safehasattr(o.events, 'e1'))
> +
> +        obj = object()
> +        o.events.register('e2', obj)
> +        self.assertTrue(safehasattr(o.events, 'e2'))
> +
> +        calls = {'e1': 0, 'e2': []}
> +
> +        def e1h():
> +            calls['e1'] += 1
> +
> +        o.events.e1 += e1h
> +        o.events.e1()
> +        self.assertEqual(calls, {'e1': 1, 'e2': []})
> +
> +        def e2h(obj2, foo, bar):
> +            self.assertEqual(obj2, obj)
> +            calls['e2'].append((foo, bar))
> +
> +        o.events.e2 += e2h
> +        o.events.e2('1', '2')
> +        self.assertEqual(calls, {'e1': 1, 'e2': [('1', '2')]})
> +
> +if __name__ == '__main__':
> +    silenttestrunner.main(__name__)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

On Mon, 2014-08-18 at 22:53 -0700, Gregory Szorc wrote:
> On 8/18/2014 10:30 PM, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1408422497 25200
> > #      Mon Aug 18 21:28:17 2014 -0700
> > # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
> > # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> > util: add event handling mechanism
> 
> > diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py
> > new file mode 100644
> > --- /dev/null
> > +++ b/tests/test-eventmanager.py
> > @@ -0,0 +1,109 @@
> > +from mercurial.util import event, eventmanager, safehasattr
> > +
> > +import unittest
> > +import silenttestrunner
> > +
> > +class testeventmanager(unittest.TestCase):
> > +    def test_eventsimple(self):
> 
> test-check-commit-hg.t doesn't like this commit because it is
> introducing a function with underscores. But I just stole this naming
> convention from other unittest-based .py tests. I'm not sure who is in
> the wrong here.

The unittest framework is wrong, as is most of the Python standard
library: just compare it to the core language.

More seriously, we DO need to enforce this somewhere, we can't enforce
it in check-code yet, and check-commit isn't remotely sophisticated
enough yet to handle exceptional cases.

On 8/19/14 11:22 PM, Matt Mackall wrote:
> On Mon, 2014-08-18 at 22:53 -0700, Gregory Szorc wrote:
>> On 8/18/2014 10:30 PM, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc@gmail.com>
>>> # Date 1408422497 25200
>>> #      Mon Aug 18 21:28:17 2014 -0700
>>> # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
>>> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
>>> util: add event handling mechanism
>>
>>> diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/test-eventmanager.py
>>> @@ -0,0 +1,109 @@
>>> +from mercurial.util import event, eventmanager, safehasattr
>>> +
>>> +import unittest
>>> +import silenttestrunner
>>> +
>>> +class testeventmanager(unittest.TestCase):
>>> +    def test_eventsimple(self):
>>
>> test-check-commit-hg.t doesn't like this commit because it is
>> introducing a function with underscores. But I just stole this naming
>> convention from other unittest-based .py tests. I'm not sure who is in
>> the wrong here.
>
> The unittest framework is wrong, as is most of the Python standard
> library: just compare it to the core language.

PEP-8 says to use functions_with_underscores not functionsasoneword. 
unittest is a bit inconsistent: unittest.TestCase has both camelCase and 
lowercase methods. I'm not sure whether this is historical or an attempt 
to distinguish and/or avoid conflicts between unittest "core" APIs and 
user/test-supplied code.

Viewed through PEP-8, Mercurial's style convention is what's "wrong."

Anyway, the default regexp in unittest for discovering test methods 
allows testwithoutunderscore. So if you tell me to remove it, I will.

mpm: I'd appreciate your feedback on the event part of this proposal. My 
extensions that override push behavior really need something like this. 
See e.g. the 2 functions starting at 
https://hg.mozilla.org/hgcustom/version-control-tools/file/d50b97f6e22f/hgext/reviewboard/client.py#l128

On Wed, 2014-08-20 at 09:16 -0700, Gregory Szorc wrote:
> On 8/19/14 11:22 PM, Matt Mackall wrote:
> > On Mon, 2014-08-18 at 22:53 -0700, Gregory Szorc wrote:
> >> On 8/18/2014 10:30 PM, Gregory Szorc wrote:
> >>> # HG changeset patch
> >>> # User Gregory Szorc <gregory.szorc@gmail.com>
> >>> # Date 1408422497 25200
> >>> #      Mon Aug 18 21:28:17 2014 -0700
> >>> # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b
> >>> # Parent  8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a
> >>> util: add event handling mechanism
> >>
> >>> diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py
> >>> new file mode 100644
> >>> --- /dev/null
> >>> +++ b/tests/test-eventmanager.py
> >>> @@ -0,0 +1,109 @@
> >>> +from mercurial.util import event, eventmanager, safehasattr
> >>> +
> >>> +import unittest
> >>> +import silenttestrunner
> >>> +
> >>> +class testeventmanager(unittest.TestCase):
> >>> +    def test_eventsimple(self):
> >>
> >> test-check-commit-hg.t doesn't like this commit because it is
> >> introducing a function with underscores. But I just stole this naming
> >> convention from other unittest-based .py tests. I'm not sure who is in
> >> the wrong here.
> >
> > The unittest framework is wrong, as is most of the Python standard
> > library: just compare it to the core language.
> 
> PEP-8 says to use functions_with_underscores not functionsasoneword. 
> unittest is a bit inconsistent: unittest.TestCase has both camelCase and 
> lowercase methods. I'm not sure whether this is historical or an attempt 
> to distinguish and/or avoid conflicts between unittest "core" APIs and 
> user/test-supplied code.
> 
> Viewed through PEP-8, Mercurial's style convention is what's "wrong."

Please do "pydoc list|dict|file|str|etc" and count the separators. Also
look at dir(__builtins__). It's clear that Python's core style is no
separator. About the only common exception to the rule in the core
language is dict.has_key.. and that got removed in Py3.

The actual PEP8 recommendation is:

"Function names should be lowercase, with words separated by underscores
as necessary to improve readability."

The second clause has that vague "as necessary" which in practice means
"when random developer feels like it". This was a mistake, because they
ended up sacrificing a huge amount of consistency (aka writability) for
a pretty small amount of readability. And it even allows monstrosities
like "set_debuglevel" because it doesn't even suggest you should be
consistent inside a single identifier! They should have chose one way or
the other, always... and the obvious choice would have been _to match
the language itself_.

So I recommend reading that "as necessary" as "never".