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.

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

Consider renaming emitter in EventsBase #82

Closed
dshcherb opened this issue Dec 11, 2019 · 4 comments
Closed

Consider renaming emitter in EventsBase #82

dshcherb opened this issue Dec 11, 2019 · 4 comments

Comments

@dshcherb
Copy link
Contributor

emitter parameter name has a different meaning in EventsBase (parent, e.g. CharmBase instance) and BoundEvent (EventsBase instance).

@dshcherb
Copy link
Contributor Author

@johnsca @jameinel

@dshcherb dshcherb changed the title Consider renaming emitter in EventsBase` Consider renaming emitter in EventsBase Dec 11, 2019
@johnsca
Copy link
Contributor

johnsca commented Dec 11, 2019

Specifically in:

operator/ops/framework.py

Lines 221 to 230 in 032b274

def __init__(self, parent=None, key=None):
if parent is not None:
super().__init__(parent, key)
def __get__(self, emitter, emitter_type):
# Same type, different instance, more data. Doing this unusual construct
# means people can subclass just this one class to have their own 'on'.
if emitter is None:
return self
return type(self)(emitter)

The emitter param to __get__ is actually the parent instance and is in fact only used to be passed into __init__ as the parent param. I vote for changing that from emitter to parent (and emitter_type to parent_type).

@johnsca
Copy link
Contributor

johnsca commented Dec 12, 2019

It's also worth noting that the EventsBase instance (the on) is the actual emitter in that case, not the charm instance, but in the getter, the emitter var points to the charm instance.

@niemeyer
Copy link
Collaborator

The name of the emitter variable is semantic in the context of EventsBase. Every framework Object may have a parent. But this is an EventsBase, supposed to handle boilerplate for emitting events for a certain object. Which object? The emitter, which we receive via the decorator protocol on the __get__ method. As such, the fact we want an emitter as the parameter of that function is relevant, and makes the code more readable than if it was just another parent.

Looking at BoundEvent, it seems to me that the exact same is true. The emitter there is the object emitting events.

Please do raise the issue again if that's off in any way. For now, I'm tentatively closing the issue as it all seems to make sense.

tonyandrewmeyer pushed a commit to tonyandrewmeyer/operator that referenced this issue Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants