[PATCH 1 of 6 events v3] util: add event handling mechanism

Gregory Szorc gregory.szorc at gmail.com
Mon Sep 29 17:44:57 CDT 2014


On 9/29/14 3:24 PM, Mads Kiilerich wrote:
> On 09/28/2014 11:01 PM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc at gmail.com>
>> # Date 1411933407 25200
>> #      Sun Sep 28 12:43:27 2014 -0700
>> # Node ID d513c232aeba16137b291456536adee0d88a040f
>> # Parent  4109cc16279ef0e04dc70e7f4c9ab7415e6a22d3
>> util: add event handling mechanism
>>
>> The patch adds a generic event handling mechanism to Mercurial. From a
>> high level, you create a class with methods corresponding to event
>> names. You can then register functions/callbacks against events that
>> get called when that event fires.
>>
>> 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 a more
>> well-defined point for code insertion.
>
> That sounds a bit like aspect-oriented programming, while events can be
> many other things. Why is not called AOP and why is it called events?

I don't care what color we paint the bike shed. Give me a better 
nomenclature and I'll update the patch series.

>> Events will provide a better mechanism for code insertion. Events solve
>> the discovery problem by providing a well-defined (like hooks) set of
>> places for supported code insertion. Events are also easier to code,
>> as extension authors don't need to worry about monkeypatching: just
>> write a function and register it.
>
> That might be true, but this patch series do not provide any evidence of
> what improvements it actually could bring ... or that it is a good
> mechanism for this kind of improvements. Actual use of this mechanism in
> the built-in extensions would help a lot to validate it. Without actual
> use it just adds quite a bit of complexity and no benefits.

The thing is that I wrote this patch series to solve problems that I as 
an extension author have come across. The intended use case for this 
feature is mostly extensions. I thought I had mpm's buy-in at the sprint 
that this was a good idea.

If you insist, I could find use cases inside core to make use of events.

>> +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
>> by defining
>> +    methods on ``eventmanager`` instances.
>> +
>> +    Handler functions can be registered against an instance via the
>> ``+=``
>> +    operator. They can be unregistered via the ``-=`` operator.
>> +
>> +    Handler functions can be invoked by calling an ``event`` instance
>> like
>> +    it is a function.
>> +
>> +    Handlers are executed in the order they are registered.
>> +
>> +    The return value of handler functions is ignored.
>
> I guess this imply that it should be "pure" functions that can't have
> side effects (or any effect at all?) unless the functions are curryed
> with some context? But they are allowed to raise exceptions? Is it
> accepted or frowned upon to modify the parameters?

Behavior depends on the event. The pre-push events could modify the 
pushop, for example.

Currently, we don't do any exception catching anywhere. This could 
presumably be extended so individual call sites catch and react to 
certain exceptions. For example, we could provide events that give 
extensions a well-defined location to abort the current procedure.

>> +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.
>> +
>> +    This class is an abstract base class. Actual event containers should
>> +    subclass this class. Events are registered by defining methods on
>> these
>> +    subclasses. The methods should define the named arguments the event
>> +    accepts and a docstring describing the purpose of the event. The
>> custom
>> +    __new__ implementation of this class will automagically convert each
>> +    method to an ``event`` instance.
>> +
>> +    Methods lacking docstrings will result in an exception during class
>> +    creation. This requirement serves to reinforce the well-defined
>> intent
>> +    of event APIs.
>
> -1 to enforcing it just to enforce it. Mandatory docstrings is a policy
> - that should not be hardcoded.

You disagree with marmoute :/

Personally, I find machines are better at enforcing policies than 
humans. Hence this code. I suppose I could move this to a test...



More information about the Mercurial-devel mailing list