Remove the need for explicit event decoration

[johnsca]

By splitting Event into the more explicit UnassignedEvent and AssignedEvent to go along with BoundEvent, and by using a metaclass for the unassigned state to allow the descriptor pattern to work at the class level, it's possible to use the event type directly and avoid the need for the charm author to explicitly decorate the event every time it is used, while reducing the potential for conflicts and keeping the distinction between the event type, bound state, and event instances well separated.

The only change in usage is that instead of:

class CharmEvents(EventsBase):
    install = Event(InstallEvent)

it is defined as:

class CharmEvents(EventsBase):
    install = InstallEvent

The event is still emitted in the same way:

    # self.on.install is still a BoundEvent
    self.on.install.emit()

[niemeyer]

This is actually the metaclass of the event. Assigned, unassigned, or whatever else happens, it's still its metaclass. It's also a single line long, and never going to be used directly by developers, so no need to document it in that style. Accordingly, its name should indicate what it really is. I suggest EventMetaType.

[dshcherb]

Following what's going on when 2 Python features (__set_name__ and descriptors) are involved is a bit difficult without either helpful naming or a docstring in my view.

By glancing over at names at first I could not tell the difference between Assigned and Bound.

If I read out loud what's going on:

• Assigning a subclass of UnassignedEvent to a class attribute will instead result in an assignment of an event descriptor to that attribute;
• When that attribute is then accessed and the descriptor's __get__ method is invoked, an instance of BoundEvent is returned which can then be used to emit events tied to an emitter type.

I think it should be clear that we aim to bind "unbound" event types to another type. This is done via descriptors so EventDescriptor sounds like a good name.

UnboundEvent instead of UnassignedEvent correlates with BoundEvent and makes the intention a bit clearer to me.

EventMetaType would also work so long as there is an explanation about the required binding in the docstring.

[johnsca]

I went with EventMetaType as suggested and a hopefully short but useful doc string.

[niemeyer]

This is the actual meat of this PR. This line replacing Event(SomeEvent) by deep Python magic (metaclasses, special methods, etc) that doesn't make sense until we dig down. I'm usually on the fence about these things, as it'll obscure code that people will be typing themselves.. this:

class C:
     foo = Bar

doesn't do what it says on the tin anymore. Literally, C.foo is not Bar.

With that said, we can certainly give it a shot and see how people feel in practice. But this PR is doing a lot more than that, which is not great. What pieces of this PR are indeed required to achieve this goal, vs. things which are irrelevant for this change?

How about this: AssignedEvent is really our current Event type. I think we can be precise and name it as EventDescriptor now that we're not typing it all out anymore. But let's not do that now.. let's keep it named Event, and try not to touch it. With that, and this metaclass, what else do we need on this PR for this change to work out?

[johnsca]

I definitely see the point that this replaces the explicit wrapping of the decorator with implicit behavior that might be surprising, but I think that it ends up being worth it when weighed against the possibility that a new charm author might not realize they need to always wrap the event type with the Event descriptor, or even that an experienced charm author might forget.

At the end of the day, I think the charm author really will care about defining an event and having self.on.foo.emit() work and not so much about how that ends up working under the hood. There is also a parallel to how instance methods work in Python; like instance methods, having events become bound is the common case and thus should be the default, just like there is no @instancemethod decorator required in Python and the binding just happens automatically because that's almost always what you intend.

[johnsca]

I'm definitely +1 to renaming this to EventDescriptor in a future PR to avoid confusion.

[johnsca]

Per discussion, it seems like we're leaning to finding a better name for Event and keeping it explicit. Leaving the PR open for the moment because we didn't come up with a compelling name, but I expect it to be closed eventually.

[niemeyer]

After much discussion, we have agreement inside the team that code like this:

class SomeClass:
       foo = SomeEvent
       bar = SomeEvent
       baz = SomeEvent

Doesn't read properly in Python, even if the underlying metaclass magic is replacing these types by the actual descriptor which is what we do explicitly today. This, instead, makes it both more explicit and more readable:

class SomeClass:
       foo = EventDescriptor(SomeEvent)
       bar = EventDescriptor(SomeEvent)
       baz = EventDescriptor(SomeEvent)

We don't like the length of that name, though, but it would be the most explicit and most semantically correct. We'll be looking for a better name for this.

Closing this PR as we have agreement not to replace the descriptor by the metaclass magic.

[niemeyer]

We agreed on EventSource as the term, being pushed forward via #91.