Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the need for explicit event decoration #78

Closed
wants to merge 3 commits into from
Closed

Remove the need for explicit event decoration #78

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b85df78
Remove the need for explicit event decoration
johnsca Dec 10, 2019
46f5e87
Minimize changes and rename classes per review
johnsca Dec 11, 2019
eb2644a
Remove unnecessary comment addition
johnsca Dec 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions ops/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from ops.framework import Object, Event, EventBase, EventsBase
from ops.framework import Object, EventBase, EventsBase


class HookEvent(EventBase):
Expand Down Expand Up @@ -113,16 +113,16 @@ class StorageDetachingEvent(StorageEvent):

class CharmEvents(EventsBase):

install = Event(InstallEvent)
start = Event(StartEvent)
stop = Event(StopEvent)
update_status = Event(UpdateStatusEvent)
config_changed = Event(ConfigChangedEvent)
upgrade_charm = Event(UpgradeCharmEvent)
pre_series_upgrade = Event(PreSeriesUpgradeEvent)
post_series_upgrade = Event(PostSeriesUpgradeEvent)
leader_elected = Event(LeaderElectedEvent)
leader_settings_changed = Event(LeaderSettingsChangedEvent)
install = InstallEvent
start = StartEvent
stop = StopEvent
update_status = UpdateStatusEvent
config_changed = ConfigChangedEvent
upgrade_charm = UpgradeCharmEvent
pre_series_upgrade = PreSeriesUpgradeEvent
post_series_upgrade = PostSeriesUpgradeEvent
leader_elected = LeaderElectedEvent
leader_settings_changed = LeaderSettingsChangedEvent


class CharmBase(Object):
Expand Down
54 changes: 26 additions & 28 deletions ops/framework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,16 @@ def from_path(cls, path):
return handle


class EventBase:
class EventMetaType(type):
"""Metaclass for event types which automatically applies the Event descriptor when the
event type is assigned in an emitter class definition.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def __set_name__(event_type, emitter_type, event_kind):
setattr(emitter_type, event_kind, Event(event_type, event_kind, emitter_type))


class EventBase(metaclass=EventMetaType):

def __init__(self, handle):
self.handle = handle
Expand All @@ -115,32 +124,16 @@ def restore(self, snapshot):
class Event:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Event creates class descriptors to operate with events.

It is generally used as:

class SomethingHappened(EventBase):
pass

class SomeObject:
something_happened = Event(SomethingHappened)


With that, instances of that type will offer the someobj.something_happened
attribute which is a BoundEvent and may be used to emit and observe the event.
It is automatically applied by the EventMetaType and causes the event types to
be wrapped in BoundEvents when accessed from an emitter instance. The BoundEvent
wrapper allows the event to be emitted and observed via the emitter instance's
framework reference.
"""

def __init__(self, event_type):
def __init__(self, event_type, event_kind, emitter_type):
if not isinstance(event_type, type) or not issubclass(event_type, EventBase):
raise RuntimeError(f"Event requires a subclass of EventBase as an argument, got {event_type}")
self.event_type = event_type
self.event_kind = None
self.emitter_type = None

def __set_name__(self, emitter_type, event_kind):
if self.event_kind is not None:
raise RuntimeError(
f'Event({self.event_type.__name__}) reused as '
f'{self.emitter_type.__name__}.{self.event_kind} and '
f'{emitter_type.__name__}.{event_kind}')
self.event_kind = event_kind
self.emitter_type = emitter_type

Expand All @@ -149,8 +142,14 @@ def __get__(self, emitter, emitter_type=None):
return self
return BoundEvent(emitter, self.event_type, self.event_kind)

def __repr__(self):
return (f'<Event {self.event_type.__name__} assigned as '
f'{self.event_kind} to {self.emitter_type} '
f'at {hex(id(self))}')


class BoundEvent:
"""An event bound to an emitter."""

def __repr__(self):
return (f'<BoundEvent {self.event_type.__name__} bound to '
Expand Down Expand Up @@ -202,7 +201,7 @@ def __init__(self, parent, key):

# TODO This can probably be dropped, because the event type is only
# really relevant if someone is either emitting the event or observing
# it.
# it. Specifically, this could be done in Event.__get__ instead.
for attr_name, attr_value in inspect.getmembers(type(self)):
if isinstance(attr_value, Event):
event_type = attr_value.event_type
Expand Down Expand Up @@ -247,8 +246,7 @@ def define_event(cls, event_kind, event_type):
except AttributeError:
pass

event_descriptor = Event(event_type)
event_descriptor.__set_name__(cls, event_kind)
event_descriptor = Event(event_type, event_kind, cls)
setattr(cls, event_kind, event_descriptor)

def events(self):
Expand Down Expand Up @@ -289,8 +287,8 @@ class CommitEvent(EventBase):


class FrameworkEvents(EventsBase):
pre_commit = Event(PreCommitEvent)
commit = Event(CommitEvent)
pre_commit = PreCommitEvent
commit = CommitEvent


class NoSnapshotError(Exception):
Expand Down Expand Up @@ -594,7 +592,7 @@ class StoredStateChanged(EventBase):


class StoredStateEvents(EventsBase):
changed = Event(StoredStateChanged)
changed = StoredStateChanged


class StoredStateData(Object):
Expand Down
4 changes: 2 additions & 2 deletions test/test_charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from ops.charm import CharmBase, CharmMeta
from ops.charm import CharmEvents
from ops.framework import Framework, Event, EventBase
from ops.framework import Framework, EventBase
from ops.model import Model, ModelBackend


Expand All @@ -29,7 +29,7 @@ class CustomEvent(EventBase):
pass

class TestCharmEvents(CharmEvents):
custom = Event(CustomEvent)
custom = CustomEvent

# Relations events are defined dynamically and modify the class attributes.
# We use a subclass temporarily to prevent these side effects from leaking.
Expand Down
83 changes: 49 additions & 34 deletions test/test_framework.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from pathlib import Path

from ops.framework import (
Framework, Handle, Event, EventsBase, EventBase, Object, PreCommitEvent, CommitEvent,
Framework, Handle, EventsBase, EventBase, Object, PreCommitEvent, CommitEvent,
NoSnapshotError, StoredState, StoredList, BoundStoredState, StoredStateData
)

Expand Down Expand Up @@ -110,9 +110,9 @@ class MyEvent(EventBase):
pass

class MyNotifier(Object):
foo = Event(MyEvent)
bar = Event(MyEvent)
baz = Event(MyEvent)
foo = MyEvent
bar = MyEvent
baz = MyEvent

class MyObserver(Object):
def __init__(self, parent, key):
Expand Down Expand Up @@ -150,10 +150,10 @@ class MyEvent(EventBase):
pass

class MyNotifier(Object):
foo = Event(MyEvent)
bar = Event(MyEvent)
baz = Event(MyEvent)
qux = Event(MyEvent)
foo = MyEvent
bar = MyEvent
baz = MyEvent
qux = MyEvent

class MyObserver(Object):
def on_foo(self):
Expand Down Expand Up @@ -231,11 +231,11 @@ class MyEvent(EventBase):
pass

class MyNotifier1(Object):
a = Event(MyEvent)
b = Event(MyEvent)
a = MyEvent
b = MyEvent

class MyNotifier2(Object):
c = Event(MyEvent)
c = MyEvent

class MyObserver(Object):
def __init__(self, parent, key):
Expand Down Expand Up @@ -305,7 +305,7 @@ def restore(self, snapshot):
self.my_n = snapshot["My N!"] + 1

class MyNotifier(Object):
foo = Event(MyEvent)
foo = MyEvent

class MyObserver(Object):
def __init__(self, parent, key):
Expand Down Expand Up @@ -346,7 +346,7 @@ class MyEvent(EventBase):
pass

class MyEvents(EventsBase):
foo = Event(MyEvent)
foo = MyEvent

class MyNotifier(Object):
on = MyEvents()
Expand Down Expand Up @@ -375,7 +375,7 @@ class MyEvent(EventBase):
pass

class MyEvents(EventsBase):
foo = Event(MyEvent)
foo = MyEvent

class MyNotifier(Object):
on = MyEvents()
Expand All @@ -398,29 +398,44 @@ def on_foo(self, event):

self.assertEqual(obs.seen, ["on_foo:foo"])

def test_conflicting_event_attributes(self):
def test_reused_event_types(self):
class MyEvent(EventBase):
pass

event = Event(MyEvent)
event = MyEvent

class MyEvents(EventsBase):
foo = event

with self.assertRaises(RuntimeError) as cm:
class OtherEvents(EventsBase):
foo = event
self.assertEqual(
str(cm.exception.__cause__),
"Event(MyEvent) reused as MyEvents.foo and OtherEvents.foo")

with self.assertRaises(RuntimeError) as cm:
class MyNotifier(Object):
on = MyEvents()
bar = event
self.assertEqual(
str(cm.exception.__cause__),
"Event(MyEvent) reused as MyEvents.foo and MyNotifier.bar")
class SubEvents(MyEvents):
bar = MyEvent

class OtherEvents(EventsBase):
qux = event

class MyCharm(Object):
on = SubEvents()
other = OtherEvents()
ham = event

def __init__(self, parent, key):
super().__init__(parent, key)
self.framework.observe(self.on.foo, self.on_any)
self.framework.observe(self.on.bar, self.on_any)
self.framework.observe(self.other.qux, self.on_any)
self.framework.observe(self.ham, self.on_any)
self.seen = []

def on_any(self, event):
self.seen.append(event.handle.path)

framework = self.create_framework()
charm = MyCharm(framework, None)
charm.on.foo.emit()
charm.on.bar.emit()
charm.other.qux.emit()
charm.ham.emit()
self.assertEqual(charm.seen, ['MyCharm/on/foo[1]', 'MyCharm/on/bar[2]', 'MyCharm/on/qux[3]', 'MyCharm/ham[4]'])

def test_reemit_ignores_unknown_event_type(self):
# The event type may have been gone for good, and nobody cares,
Expand All @@ -432,7 +447,7 @@ class MyEvent(EventBase):
pass

class MyNotifier(Object):
foo = Event(MyEvent)
foo = MyEvent

class MyObserver(Object):
def __init__(self, parent, key):
Expand Down Expand Up @@ -474,11 +489,11 @@ class MyBar(EventBase):
pass

class MyEvents(EventsBase):
foo = Event(MyFoo)
foo = MyFoo

class MyNotifier(Object):
on = MyEvents()
bar = Event(MyBar)
bar = MyBar

class MyObserver(Object):
def __init__(self, parent, key):
Expand Down Expand Up @@ -588,7 +603,7 @@ def restore(self, value):
self.value = value

class MyNotifier(Object):
foo = Event(MyEvent)
foo = MyEvent

class MyObserver(Object):
has_deferred = False
Expand Down