Skip to content

Commit

Permalink
Address review comments.
Browse files Browse the repository at this point in the history
* Assume that the application will always be provided for relation events, even though there is an open Juju bug that indicates that may not always be the case at the moment.
* Minor style fixes.
* Don't adjust the types in the , as charmers may be relying on the default value in tests, and there's no valid default value other than None.
  • Loading branch information
tonyandrewmeyer committed Oct 5, 2023
1 parent b6f6c42 commit 737bf0e
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 36 deletions.
57 changes: 22 additions & 35 deletions ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -369,12 +369,12 @@ class RelationEvent(HookEvent):
"""

relation: 'Relation'
"""The relation involved in this event."""
"""The relation involved in this event.
"""

app: Optional[model.Application]
app: model.Application
"""The remote application that has triggered this event.
This may be ``None`` for relation-broken events."""
"""

unit: Optional[model.Unit]
"""The remote unit that has triggered this event.
Expand All @@ -393,7 +393,12 @@ def __init__(self, handle: 'Handle', relation: 'Relation',
f'cannot create RelationEvent with application {app} and unit {unit}')

self.relation = relation
self.app = app
if app is None:
logger.warning("'app' expected but not received.")
# Do an explicit assignment here so that we can contain the type: ignore.
self.app = None # type: ignore
else:
self.app = app
self.unit = unit

def snapshot(self) -> Dict[str, Any]:
Expand Down Expand Up @@ -428,7 +433,8 @@ def restore(self, snapshot: Dict[str, Any]):
if app_name:
self.app = self.framework.model.get_app(app_name)
else:
self.app = None
logger.warning("'app_name' expected in snapshot but not found.")
self.app = None # type: ignore

unit_name = snapshot.get('unit_name')
if unit_name:
Expand All @@ -444,15 +450,9 @@ class RelationCreatedEvent(RelationEvent):
can occur before units for those applications have started. All existing
relations should be established before start.
"""
app: model.Application
"""The remote application that has triggered this event."""

unit: None
"""Always ``None``."""

def __init__(self, handle: 'Handle', relation: 'Relation',
app: model.Application, unit: None = None):
super().__init__(handle, relation, app, unit)
"""Always ``None``.
"""


class RelationJoinedEvent(RelationEvent):
Expand All @@ -466,15 +466,9 @@ class RelationJoinedEvent(RelationEvent):
remote ``private-address`` setting, which is always available when
the relation is created and is by convention not deleted.
"""
app: model.Application
"""The remote application that has triggered this event."""

unit: model.Unit
"""The remote unit that has triggered this event."""

def __init__(self, handle: 'Handle', relation: 'Relation',
app: model.Application, unit: model.Unit):
super().__init__(handle, relation, app, unit)
"""The remote unit that has triggered this event.
"""


class RelationChangedEvent(RelationEvent):
Expand All @@ -495,8 +489,6 @@ class RelationChangedEvent(RelationEvent):
The settings that may be queried, or set, are determined by the relation’s
interface.
"""
app: model.Application
"""The remote application that has triggered this event."""


class RelationDepartedEvent(RelationEvent):
Expand All @@ -517,15 +509,13 @@ class RelationDepartedEvent(RelationEvent):
Once all callback methods bound to this event have been run for such a
relation, the unit agent will fire the :class:`RelationBrokenEvent`.
"""
app: model.Application
"""The remote application that has triggered this event."""

unit: model.Unit
"""The remote unit that has triggered this event."""
"""The remote unit that has triggered this event.
"""

def __init__(self, handle: 'Handle', relation: 'Relation',
app: model.Application,
unit: model.Unit,
app: Optional[model.Application] = None,
unit: Optional[model.Unit] = None,
departing_unit_name: Optional[str] = None):
super().__init__(handle, relation, app=app, unit=unit)

Expand Down Expand Up @@ -578,11 +568,8 @@ class RelationBrokenEvent(RelationEvent):
are currently known locally.
"""
unit: None
"""Always ``None``."""

def __init__(self, handle: 'Handle', relation: 'Relation',
app: Optional[model.Application] = None, unit: None = None):
super().__init__(handle, relation, app, unit)
"""Always ``None``.
"""


class StorageEvent(HookEvent):
Expand Down
2 changes: 1 addition & 1 deletion test/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ def on_any_relation(self, event: ops.RelationEvent):
charm.on['pro1'].relation_departed.emit(rel, app, unit)
charm.on['pro-2'].relation_departed.emit(rel, app, unit)
charm.on['peer1'].relation_broken.emit(rel, app)
charm.on['peer-2'].relation_broken.emit(rel)
charm.on['peer-2'].relation_broken.emit(rel, app)

self.assertEqual(charm.seen, [
'RelationJoinedEvent',
Expand Down

0 comments on commit 737bf0e

Please sign in to comment.