Merge branch 'main' into exec-context

.github/pull_request_template.md

@@ -1,35 +1 @@
-## Please provide the following details to expedite review (and delete this heading)
-
-*Replace with a description about why this change is needed, along with a description of what changed.*
-
-## Checklist
-
- - [ ] Have any types changed? If so, have the type annotations been updated?
- - [ ] If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?
-
-       We want to avoid simple passthrough of model data, without contextualization, where possible.
-
- - [ ] Do error messages, if any, present charm authors or operators with enough information to solve the problem?
-
-## QA steps
-
-*Please replace with how we can verify that the change works.*
-
-```sh
-QA steps here
-```
-
-## Documentation changes
-
-*Please replace with any notes about how it affects current user workflow, CLI, or API.*
-
-## Bug reference
-
-*Please add a link to any bugs that this change is related to, e.g., https://bugs.launchpad.net/juju/+bug/9876543*
-
-## Changelog
-
-*Please include a bulleted list of items here, suitable for inclusion in a release changelog.*
-
-- *Adds feature foo, which allows charm authors to ...*
-- *Fixes bug bar, which means that operators no longer need workaround qux ...*
+Please describe the *why* and the *how* of your change here.

ops/framework.py

@@ -148,7 +148,7 @@ def key(self) -> str:
         return self._key
 
     @property
-    def path(self):
+    def path(self) -> str:
         """Return the handle's path."""
         return self._path
 
@@ -174,16 +174,18 @@ def from_path(cls, path: str) -> 'Handle':
 
 
 class EventBase:
-    """The base for all the different Events.
+    """The base class for all events.
 
-    Inherit this and override 'snapshot' and 'restore' methods to build a custom event.
+    Inherit this and override the ``snapshot`` and ``restore`` methods to
+    create a custom event.
     """
 
     # gets patched in by `Framework.restore()`, if this event is being re-emitted
     # after being loaded from snapshot, or by `BoundEvent.emit()` if this
     # event is being fired for the first time.
     # TODO this is hard to debug, this should be refactored
     framework: 'Framework' = None  # type: ignore
+    """The :class:`Framework` instance (set by the framework itself)."""
 
     def __init__(self, handle: Handle):
         self.handle = handle
@@ -192,7 +194,7 @@ def __init__(self, handle: Handle):
     def __repr__(self):
         return f"<{self.__class__.__name__} via {self.handle}>"
 
-    def defer(self):
+    def defer(self) -> None:
         """Defer the event to the future.
 
         Deferring an event from a handler puts that handler into a queue, to be
@@ -263,16 +265,17 @@ def restore(self, snapshot: Dict[str, Any]):
 class EventSource:
     """EventSource wraps an event type with a descriptor to facilitate observing and emitting.
 
-    It is generally used as:
+    It is generally used as::
 
         class SomethingHappened(EventBase):
             pass
 
         class SomeObject(Object):
             something_happened = EventSource(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.
+    With that, instances of that type will offer the ``someobj.something_happened``
+    attribute which is a :class:`BoundEvent`, and may be used to emit and observe
+    the event.
     """
 
     def __init__(self, event_type: 'Type[EventBase]'):
@@ -407,7 +410,7 @@ def model(self) -> 'Model':
 
 
 class ObjectEvents(Object):
-    """Convenience type to allow defining .on attributes at class level."""
+    """Convenience type to allow defining ``.on`` attributes at class level."""
 
     handle_kind = "on"
 
@@ -431,23 +434,21 @@ def __get__(self, emitter: Object, emitter_type: 'Type[Object]'):
     def define_event(cls, event_kind: str, event_type: 'Type[EventBase]'):
         """Define an event on this type at runtime.
 
-        cls: a type to define an event on.
-
-        event_kind: an attribute name that will be used to access the
-                    event. Must be a valid python identifier, not be a keyword
-                    or an existing attribute.
-
-        event_type: a type of the event to define.
-
         Note that attempting to define the same event kind more than once will
-        raise a 'overlaps with existing type' runtime error. Ops uses a
+        raise an "overlaps with existing type" runtime error. Ops uses a
         labeling system to track and reconstruct events between hook executions
         (each time a hook runs, the Juju Agent invokes a fresh instance of ops;
         there is no ops process that persists on the host between hooks).
         Having duplicate Python objects creates duplicate labels. Overwriting a
         previously created label means that only the latter code path will be
-        run when the current event, if it does get deferred, is reemitted. This
-        is usually not what is desired, and is error-prone and ambigous.
+        run when the current event, if it does get deferred, is re-emitted. This
+        is usually not what is desired, and is error-prone and ambiguous.
+
+        Args:
+            event_kind: An attribute name that will be used to access the
+                        event. Must be a valid Python identifier, not be a keyword
+                        or an existing attribute.
+            event_type: A type of the event to define.
         """
         prefix = 'unable to define an event with event_kind that '
         if not event_kind.isidentifier():
@@ -505,17 +506,21 @@ class LifecycleEvent(EventBase):
 
 
 class PreCommitEvent(LifecycleEvent):
-    """Events that will be emitted first on commit."""
+    """Event that will be emitted first on commit."""
 
 
 class CommitEvent(LifecycleEvent):
-    """Events that will be emitted second on commit."""
+    """Event that will be emitted second on commit."""
 
 
 class FrameworkEvents(ObjectEvents):
     """Manager of all framework events."""
+
     pre_commit = EventSource(PreCommitEvent)
+    """Triggered before the :attr:`commit` event."""
+
     commit = EventSource(CommitEvent)
+    """Triggered before event data is committed to storage."""
 
 
 class NoTypeError(Exception):
@@ -544,14 +549,19 @@ class Framework(Object):
     """Main interface from the Charm to the ops library's infrastructure."""
 
     on = FrameworkEvents()  # type: ignore
+    """Used for :meth:`observe`-ing framework-specific events."""
 
     # Override properties from Object so that we can set them in __init__.
     model: 'Model' = None  # type: ignore
+    """The :class:`Model` instance for this charm."""
+
     meta: 'CharmMeta' = None  # type: ignore
+    """The charm's metadata."""
+
     charm_dir: 'pathlib.Path' = None  # type: ignore
+    """The charm project root directory."""
 
     # to help the type checker and IDEs:
-
     if TYPE_CHECKING:
         _stored: 'StoredStateData' = None  # type: ignore
         @property
@@ -615,19 +625,20 @@ def __init__(self, storage: Union[SQLiteStorage, JujuStorage],
         else:
             self._juju_debug_at: Set[str] = set()
 
-    def set_breakpointhook(self):
-        """Hook into sys.breakpointhook so the builtin breakpoint() works as expected.
+    def set_breakpointhook(self) -> Optional[Any]:
+        """Hook into ``sys.breakpointhook`` so the builtin ``breakpoint()`` works as expected.
 
         This method is called by ``main``, and is not intended to be
         called by users of the framework itself outside of perhaps
         some testing scenarios.
 
-        It returns the old value of sys.excepthook.
-
-        The breakpoint function is a Python >= 3.7 feature.
+        The ``breakpoint()`` function is a Python >= 3.7 feature.
 
         This method was added in ops 1.0; before that, it was done as
-        part of the Framework's __init__.
+        part of the Framework's ``__init__``.
+
+        Returns:
+            The old value of ``sys.breakpointhook``.
         """
         old_breakpointhook = getattr(sys, 'breakpointhook', None)
         if old_breakpointhook is not None:
@@ -636,7 +647,7 @@ def set_breakpointhook(self):
             sys.breakpointhook = self.breakpoint
         return old_breakpointhook
 
-    def close(self):
+    def close(self) -> None:
         """Close the underlying backends."""
         self._storage.close()
 
@@ -654,7 +665,7 @@ def _forget(self, obj: 'Serializable'):
         """Stop tracking the given object. See also _track."""
         self._objects.pop(obj.handle.path, None)
 
-    def commit(self):
+    def commit(self) -> None:
         """Save changes to the underlying backends."""
         # Give a chance for objects to persist data they want to before a commit is made.
         self.on.pre_commit.emit()
@@ -722,6 +733,9 @@ def drop_snapshot(self, handle: Handle):
     def observe(self, bound_event: BoundEvent, observer: Callable[[Any], None]):
         """Register observer to be called when bound_event is emitted.
 
+        If this is called multiple times for the same event type, the
+        framework calls the observers in the order they were observed.
+
         The bound_event is generally provided as an attribute of the object that emits
         the event, and is created in this style::
 
@@ -818,7 +832,7 @@ def _emit(self, event: EventBase):
         if saved:
             self._reemit(event_path)
 
-    def reemit(self):
+    def reemit(self) -> None:
         """Reemit previously deferred events to the observers that deferred them.
 
         Only the specific observers that have previously deferred the event will be
@@ -961,7 +975,7 @@ def breakpoint(self, name: Optional[str] = None):
                 "Breakpoint %r skipped (not found in the requested breakpoints: %s)",
                 name, indicated_breakpoints)
 
-    def remove_unreferenced_events(self):
+    def remove_unreferenced_events(self) -> None:
         """Remove events from storage that are not referenced.
 
         In older versions of the framework, events that had no observers would get recorded but
@@ -1073,16 +1087,16 @@ def set_default(self, **kwargs: Any):
 
 
 class StoredState:
-    """A class used to store data the charm needs persisted across invocations.
+    """A class used to store data the charm needs, persisted across invocations.
 
     Example::
 
         class MyClass(Object):
             _stored = StoredState()
 
-    Instances of `MyClass` can transparently save state between invocations by
-    setting attributes on `_stored`. Initial state should be set with
-    `set_default` on the bound object, that is::
+    Instances of ``MyClass`` can transparently save state between invocations by
+    setting attributes on ``_stored``. Initial state should be set with
+    ``set_default`` on the bound object, that is::
 
         class MyClass(Object):
             _stored = StoredState()

ops/jujuversion.py

@@ -24,19 +24,20 @@
 class JujuVersion:
     """Helper to work with the Juju version.
 
-    It knows how to parse the ``JUJU_VERSION`` environment variable, and exposes different
-    capabilities according to the specific version, allowing also to compare with other
-    versions.
+    It knows how to parse the ``JUJU_VERSION`` environment variable, and
+    exposes different capabilities according to the specific version. It also
+    allows users to compare ``JujuVersion`` instances with ``<`` and ``>``
+    operators.
     """
 
-    PATTERN = r'''^
+    _pattern_re = re.compile(r'''^
     (?P<major>\d{1,9})\.(?P<minor>\d{1,9})       # <major> and <minor> numbers are always there
     ((?:\.|-(?P<tag>[a-z]+))(?P<patch>\d{1,9}))? # sometimes with .<patch> or -<tag><patch>
     (\.(?P<build>\d{1,9}))?$                     # and sometimes with a <build> number.
-    '''
+    ''', re.VERBOSE)
 
     def __init__(self, version: str):
-        m = re.match(self.PATTERN, version, re.VERBOSE)
+        m = self._pattern_re.match(version)
         if not m:
             raise RuntimeError(f'"{version}" is not a valid Juju version string')
 
@@ -95,27 +96,27 @@ def __lt__(self, other: 'JujuVersion') -> bool:
 
     @classmethod
     def from_environ(cls) -> 'JujuVersion':
-        """Build a JujuVersion from JUJU_VERSION."""
+        """Build a version from the ``JUJU_VERSION`` environment variable."""
         v = os.environ.get('JUJU_VERSION')
         if v is None:
             v = '0.0.0'
         return cls(v)
 
     def has_app_data(self) -> bool:
-        """Determine whether this Juju version knows about app data."""
+        """Report whether this Juju version supports app data."""
         return (self.major, self.minor, self.patch) >= (2, 7, 0)
 
     def is_dispatch_aware(self) -> bool:
-        """Determine whether this Juju version knows about dispatch."""
+        """Report whether this Juju version supports dispatch."""
         return (self.major, self.minor, self.patch) >= (2, 8, 0)
 
     def has_controller_storage(self) -> bool:
-        """Determine whether this Juju version supports controller-side storage."""
+        """Report whether this Juju version supports controller-side storage."""
         return (self.major, self.minor, self.patch) >= (2, 8, 0)
 
     @property
     def has_secrets(self) -> bool:
-        """Determine whether this Juju version supports the `secrets` feature."""
+        """Report whether this Juju version supports the "secrets" feature."""
         # Juju version 3.0.0 had an initial version of secrets, but:
         # * In 3.0.2, secret-get "--update" was renamed to "--refresh", and
         #   secret-get-info was separated into its own hook tool