Docstrings and minor updates (just tests left)

ops/__init__.py

@@ -102,7 +102,7 @@
     'FrameworkEvents',
     'Handle',
     'HandleKind',
-    'LifecycleEvent',  # TODO: this probably shouldn't be public
+    'LifecycleEvent',
     'NoTypeError',
     'Object',
     'ObjectEvents',

ops/charm.py

@@ -827,17 +827,71 @@ def defer(self) -> None:
 
 
 class CollectStatusEvent(EventBase):
-    """TODO."""
+    """Event triggered at the end of every hook to collect statuses for evaluation.
+
+    If the charm wants to provide application or unit status in a consistent
+    way after the end of every hook, it should observe the
+    :attr:`collect_app_status <CharmEvents.collect_app_status>` or
+    :attr:`collect_unit_status <CharmEvents.collect_unit_status>` event,
+    respectively.
+
+    The framework will trigger these events after the hook code runs
+    successfully, and if any statuses have been added using :meth:`add_status`,
+    the framework will choose the highest-priority status and set that as the
+    status (application status for ``collect_app_status``, or unit status for
+    ``collect_unit_status``).
+
+    The order of priorities is as follows, from highest to lowest:
+
+    * error
+    * blocked
+    * waiting
+    * maintenance
+    * active
+    * unknown
+
+    If there are multiple statuses with the same priority, the first one added
+    wins (and if an event is observed multiple times, the handlers are called
+    in the order they were observed).
+
+    A collect-status event can be observed multiple times, and
+    :meth:`add_status` can be called multiple times to add multiple statuses
+    for evaluation. This is useful when a charm has multiple components that
+    each have a status. Each code path in a collect-status handler should
+    call ``add_status`` at least once.
+
+    Below is an example "web app" charm component that observes
+    ``collect_unit_status`` to provide the status of the component, which
+    requires a "port" config option set before it can proceed::
+
+        class MyCharm(ops.CharmBase):
+            def __init__(self, *args):
+                super().__init__(*args)
+                self.webapp = Webapp(self)
+                # initialize other components
+
+        class WebApp(ops.Object):
+            def __init__(self, charm: ops.CharmBase):
+                super().__init__(charm, 'webapp')
+                self.framework.observe(charm.on.collect_unit_status, self._on_collect_status)
+
+            def _on_collect_status(self, event: ops.CollectStatusEvent):
+                if 'port' not in self.model.config:
+                    event.add_status(ops.BlockedStatus('please set "port" config'))
+                    return
+                event.add_status(ops.ActiveStatus())
+    """
 
-    def add_status(self, status: model.StatusBase) -> None:
+    def add_status(self, status: model.StatusBase):
         """Add a status for evaluation.
 
-        TODO: flesh out docstring, describe app vs unit
+        See :class:`CollectStatusEvent` for a description of how to use this.
         """
+        model = self.framework.model
         if self.handle.kind == 'collect_app_status':
-            self.framework.model._app_statuses.append(status)
+            model.app._collected_statuses.append(status)
         else:
-            self.framework.model._unit_statuses.append(status)
+            model.unit._collected_statuses.append(status)
 
 
 class CharmEvents(ObjectEvents):
@@ -896,29 +950,41 @@ class CharmEvents(ObjectEvents):
 
     leader_settings_changed = EventSource(LeaderSettingsChangedEvent)
     """DEPRECATED. Triggered when leader changes any settings (see
-    :class:`LeaderSettingsChangedEvent`)."""
+    :class:`LeaderSettingsChangedEvent`).
+    """
 
     collect_metrics = EventSource(CollectMetricsEvent)
     """Triggered by Juju to collect metrics (see :class:`CollectMetricsEvent`)."""
 
     secret_changed = EventSource(SecretChangedEvent)
     """Triggered by Juju on the observer when the secret owner changes its contents (see
-    :class:`SecretChangedEvent`)."""
+    :class:`SecretChangedEvent`).
+    """
 
     secret_expired = EventSource(SecretExpiredEvent)
     """Triggered by Juju on the owner when a secret's expiration time elapses (see
-    :class:`SecretExpiredEvent`)."""
+    :class:`SecretExpiredEvent`).
+    """
 
     secret_rotate = EventSource(SecretRotateEvent)
     """Triggered by Juju on the owner when the secret's rotation policy elapses (see
-    :class:`SecretRotateEvent`)."""
+    :class:`SecretRotateEvent`).
+    """
 
     secret_remove = EventSource(SecretRemoveEvent)
     """Triggered by Juju on the owner when a secret revision can be removed (see
-    :class:`SecretRemoveEvent`)."""
+    :class:`SecretRemoveEvent`).
+    """
 
     collect_app_status = EventSource(CollectStatusEvent)
+    """Triggered at the end of every hook to collect app statuses for evaluation
+    (see :class:`CollectStatusEvent`).
+    """
+
     collect_unit_status = EventSource(CollectStatusEvent)
+    """Triggered at the end of every hook to collect unit statuses for evaluation
+    (see :class:`CollectStatusEvent`).
+    """
 
 
 class CharmBase(Object):

ops/framework.py

@@ -625,9 +625,6 @@ def __init__(self, storage: Union[SQLiteStorage, JujuStorage],
         else:
             self._juju_debug_at: Set[str] = set()
 
-        self._app_statuses = []
-        self._unit_statuses = []
-
     def set_breakpointhook(self) -> Optional[Any]:
         """Hook into sys.breakpointhook so the builtin breakpoint() works as expected.
 

ops/main.py

@@ -440,8 +440,7 @@ def main(charm_class: Type[ops.charm.CharmBase],
 
         _emit_charm_event(charm, dispatcher.event_name)
 
-        # TODO(benhoyt): should we run this in finally block too?
-        model._finalise_statuses(charm)
+        model._evaluate_status(charm)
 
         framework.commit()
     finally:

ops/model.py

@@ -120,8 +120,6 @@ def __init__(self, meta: 'ops.charm.CharmMeta', backend: '_ModelBackend'):
         storages: Iterable[str] = meta.storages
         self._storages = StorageMapping(list(storages), self._backend)
         self._bindings = BindingMapping(self._backend)
-        self._app_statuses: 'List[StatusBase]' = []
-        self._unit_statuses: 'List[StatusBase]' = []
 
     @property
     def unit(self) -> 'Unit':
@@ -276,14 +274,15 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) -
             info = self._backend.secret_info_get(id=id, label=label)
             return Secret(self._backend, id=info.id, label=info.label)
 
-    def _finalise_statuses(self, charm: 'ops.charm.CharmBase'):
+    def _evaluate_status(self, charm: 'ops.charm.CharmBase'):
+        # Trigger collect-status events and evaluate. See CollectStatusEvent for details.
         charm.on.collect_app_status.emit()
-        if self._app_statuses:
-            self.app.status = StatusBase._get_highest_priority(self._app_statuses)
+        if self.app._collected_statuses:
+            self.app.status = StatusBase._get_highest_priority(self.app._collected_statuses)
 
         charm.on.collect_unit_status.emit()
-        if self._unit_statuses:
-            self.unit.status = StatusBase._get_highest_priority(self._unit_statuses)
+        if self.unit._collected_statuses:
+            self.unit.status = StatusBase._get_highest_priority(self.unit._collected_statuses)
 
 
 class _ModelCache:
@@ -336,6 +335,7 @@ def __init__(self, name: str, meta: 'ops.charm.CharmMeta',
         self._cache = cache
         self._is_our_app = self.name == self._backend.app_name
         self._status = None
+        self._collected_statuses: 'List[StatusBase]' = []
 
     def _invalidate(self):
         self._status = None
@@ -348,6 +348,10 @@ def status(self) -> 'StatusBase':
 
         The status of remote units is always Unknown.
 
+        You can also use the :attr:`collect_app_status <CharmEvents.collect_app_status>`
+        event if you want to evaluate and set application status consistently
+        at the end of every hook.
+
         Raises:
             RuntimeError: if you try to set the status of another application, or if you try to
                 set the status of this application as a unit that is not the leader.
@@ -487,6 +491,8 @@ def __init__(self, name: str, meta: 'ops.charm.CharmMeta',
             containers: _ContainerMeta_Raw = meta.containers
             self._containers = ContainerMapping(iter(containers), backend)
 
+        self._collected_statuses: 'List[StatusBase]' = []
+
     def _invalidate(self):
         self._status = None
 
@@ -496,6 +502,10 @@ def status(self) -> 'StatusBase':
 
         The status of any unit other than yourself is always Unknown.
 
+        You can also use the :attr:`collect_unit_status <CharmEvents.collect_unit_status>`
+        event if you want to evaluate and set unit status consistently at the
+        end of every hook.
+
         Raises:
             RuntimeError: if you try to set the status of a unit other than yourself.
             InvalidStatusError: if you try to set the status to something other than
@@ -1601,16 +1611,18 @@ def register(cls, child: Type['StatusBase']):
         return child
 
     _priorities = {
-        "error": 100,
-        "blocked": 90,
-        "waiting": 80,
-        "maintenance": 70,
-        "active": 60,
+        'error': 5,
+        'blocked': 4,
+        'waiting': 3,
+        'maintenance': 2,
+        'active': 1,
+        # 'unknown' or any other status is handled below
     }
 
     @classmethod
     def _get_highest_priority(cls, statuses: 'List[StatusBase]') -> 'StatusBase':
-        return max(statuses, key=lambda s: cls._priorities.get(s.name, 0))
+        """Return the highest-priority status from a list of statuses."""
+        return max(statuses, key=lambda status: cls._priorities.get(status.name, 0))
 
 
 @StatusBase.register

test/test_multistatus.py

@@ -12,9 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""Multi-status test charm for OP035, using Framework.observe events instead
-of regular Python functions.
-"""
+"""Multi-status test charm for OP035, using Framework.observe events."""
 
 import logging
 import typing
@@ -39,28 +37,28 @@ def setUp(self):
         self.harness.begin()
 
     def test_initial(self):
-        self.harness.model._finalise_statuses(self.harness.charm)
+        self.harness.model._evaluate_status(self.harness.charm)
         status = self.harness.model.unit.status
         self.assertEqual(status.name, "blocked")
         self.assertEqual(status.message, '"database_mode" required')
 
     def test_database_mode_set(self):
         self.harness.update_config({"database_mode": "single"})
-        self.harness.model._finalise_statuses(self.harness.charm)
+        self.harness.model._evaluate_status(self.harness.charm)
         status = self.harness.model.unit.status
         self.assertEqual(status.name, "blocked")
         self.assertEqual(status.message, '"webapp_port" required')
 
     def test_webapp_port_set(self):
         self.harness.update_config({"webapp_port": 8080})
-        self.harness.model._finalise_statuses(self.harness.charm)
+        self.harness.model._evaluate_status(self.harness.charm)
         status = self.harness.model.unit.status
         self.assertEqual(status.name, "blocked")
         self.assertEqual(status.message, '"database_mode" required')
 
     def test_all_config_set(self):
         self.harness.update_config({"database_mode": "single", "webapp_port": 8080})
-        self.harness.model._finalise_statuses(self.harness.charm)
+        self.harness.model._evaluate_status(self.harness.charm)
         status = self.harness.model.unit.status
         self.assertEqual(status.name, "active")