Clean up UpdateHooks. #329

src/reader/_types.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import logging
+from collections import defaultdict
 from collections.abc import Callable
 from collections.abc import Iterable
 from collections.abc import Mapping
@@ -13,7 +14,6 @@
 from types import MappingProxyType
 from types import SimpleNamespace
 from typing import Any
-from typing import Generic
 from typing import get_args
 from typing import Literal
 from typing import NamedTuple
@@ -614,60 +614,53 @@ def make_plugin_name(self, plugin_name: str, key: str | None = None) -> str:
 )
 
 
-UpdateHook = Callable[..., None]
-UpdateHookType = Literal[
-    'before_feeds_update',
-    'before_feed_update',
-    'after_entry_update',
-    'after_feed_update',
-    'after_feeds_update',
-]
-
-
-class UpdateHooks(dict[UpdateHookType, list[UpdateHook]], Generic[_T]):
-    def __init__(self, target: _T):
-        super().__init__()
+class UpdateHooks:
+    def __init__(self, target: Any):
         self.target = target
-
-    def __missing__(self, key: UpdateHookType) -> list[UpdateHook]:
-        return self.setdefault(key, [])
+        self.hooks: dict[str, list[Callable[..., None]]] = defaultdict(list)
 
     def run(
-        self, when: UpdateHookType, resource_id: tuple[str, ...] | None, *args: Any
-    ) -> None:
-        for hook in self[when]:
+        self,
+        when: str,
+        resource_id: tuple[str, ...] | None,
+        *args: Any,
+        return_exceptions: bool = False,
+    ) -> list[SingleUpdateHookError]:
+        rv = []
+        for hook in self.hooks[when]:
             try:
                 hook(self.target, *args)
             except Exception as e:
-                raise SingleUpdateHookError(when, hook, resource_id) from e
+                wrapper = SingleUpdateHookError(when, hook, resource_id)
+                wrapper.__cause__ = e
+                if not return_exceptions:
+                    raise wrapper
+                rv.append(wrapper)
+        return rv
 
     def group(self, message: str) -> _UpdateHookErrorGrouper:
         return _UpdateHookErrorGrouper(self, message)
 
 
 class _UpdateHookErrorGrouper:
-    def __init__(self, hooks: UpdateHooks[Any], message: str):
+    def __init__(self, hooks: UpdateHooks, message: str):
         self.hooks = hooks
         self.message = message
         self.exceptions: list[UpdateHookError] = []
         self.seen_dedupe_keys: set[Any] = set()
 
     def run(
         self,
-        when: UpdateHookType,
+        when: str,
         resource_id: tuple[str, ...] | None,
         *args: Any,
         limit: int = 0,
     ) -> None:
-        for hook in self.hooks[when]:
-            try:
-                hook(self.hooks.target, *args)
-            except Exception as e:
-                exc = SingleUpdateHookError(when, hook, resource_id)
-                exc.__cause__ = e
-                self.add(exc, resource_id, limit)
+        for exc in self.hooks.run(when, resource_id, *args, return_exceptions=True):
+            self.add(exc, resource_id, limit)
 
     def add(self, exc: UpdateHookError, dedupe_key: Any = None, limit: int = 0) -> None:
+        # TODO: test error deduping; also, the logic may not be correct?
         if limit and dedupe_key not in self.seen_dedupe_keys:  # pragma: no cover
             if len(self.seen_dedupe_keys) >= limit:
                 log.error("too many hook errors; discarding exception", exc_info=exc)
@@ -679,6 +672,15 @@ def close(self) -> None:
         if self.exceptions:
             raise UpdateHookErrorGroup(self.message, self.exceptions)
 
+    def __enter__(self) -> _UpdateHookErrorGrouper:
+        return self
+
+    def __exit__(self, _: Any, exc: BaseException, __: Any) -> None:
+        # bare SingleUpdateHookError was intended to raise, don't catch it
+        if isinstance(exc, UpdateHookErrorGroup):
+            self.add(exc)
+        self.close()
+
 
 class StorageType(Protocol):  # pragma: no cover
     r"""Storage DAO protocol.

src/reader/_update.py

@@ -483,27 +483,25 @@ def update_feed(
         # if feed_for_update.url != parsed_feed.feed.url, the feed was redirected.
         # TODO: Maybe handle redirects somehow else (e.g. change URL if permanent).
 
-        hook_errors = hooks.group("got unexpected after-update hook errors")
-
-        new_count = 0
-        updated_count = 0
-        for entry in entries:
-            if entry.new:
-                new_count += 1
-                entry_status = EntryUpdateStatus.NEW
-            else:
-                updated_count += 1
-                entry_status = EntryUpdateStatus.MODIFIED
-
-            hook_errors.run(
-                'after_entry_update',
-                entry.entry.resource_id,
-                entry.entry,
-                entry_status,
-                limit=5,
-            )
+        with hooks.group("got unexpected after-update hook errors") as hook_errors:
+            new_count = 0
+            updated_count = 0
+            for entry in entries:
+                if entry.new:
+                    new_count += 1
+                    entry_status = EntryUpdateStatus.NEW
+                else:
+                    updated_count += 1
+                    entry_status = EntryUpdateStatus.MODIFIED
+
+                hook_errors.run(
+                    'after_entry_update',
+                    entry.entry.resource_id,
+                    entry.entry,
+                    entry_status,
+                    limit=5,
+                )
 
-        hook_errors.run('after_feed_update', (url,), url)
-        hook_errors.close()
+            hook_errors.run('after_feed_update', (url,), url)
 
         return new_count, updated_count

src/reader/core.py

@@ -43,10 +43,8 @@
 from .exceptions import ParseError
 from .exceptions import PluginInitError
 from .exceptions import SearchNotEnabledError
-from .exceptions import SingleUpdateHookError
 from .exceptions import TagNotFoundError
 from .exceptions import UpdateHookError
-from .exceptions import UpdateHookErrorGroup
 from .plugins import _load_plugins
 from .plugins import DEFAULT_PLUGINS
 from .plugins import PluginInput
@@ -868,8 +866,7 @@ def update_feeds(
             instead of :attr:`~Feed.last_updated`.
 
         """
-        hook_errors = self._update_hooks.group("some hooks failed")
-        try:
+        with self._update_hooks.group("some hooks failed") as hook_errors:
             results = self.update_feeds_iter(
                 feed=feed,
                 tags=tags,
@@ -897,18 +894,6 @@ def update_feeds(
 
                 assert not isinstance(value, Exception), value
 
-        except SingleUpdateHookError as e:
-            assert e.when == 'before_feeds_update', e
-            raise
-
-        except UpdateHookErrorGroup as e:
-            for exc in e.exceptions:
-                assert isinstance(exc, SingleUpdateHookError), exc
-                assert exc.when == 'after_feeds_update', exc
-            hook_errors.add(e)
-
-        hook_errors.close()
-
     def update_feeds_iter(
         self,
         *,
@@ -1030,11 +1015,10 @@ def update_feeds_iter(
             yield from Pipeline(self, now, map).update(filter)
 
         if _call_feeds_update_hooks:
-            hook_errors = self._update_hooks.group(
+            with self._update_hooks.group(
                 "got unexpected after-update hook errors"
-            )
-            hook_errors.run('after_feeds_update', None)
-            hook_errors.close()
+            ) as hook_errors:
+                hook_errors.run('after_feeds_update', None)
 
     def update_feed(self, feed: FeedInput, /) -> UpdatedFeed | None:
         r"""Update a single feed.
@@ -2223,7 +2207,7 @@ def before_feeds_update_hooks(self) -> MutableSequence[FeedsUpdateHook]:
             Wrap unexpected exceptions in :exc:`UpdateHookError`.
 
         """
-        return self._update_hooks['before_feeds_update']
+        return self._update_hooks.hooks['before_feeds_update']
 
     @property
     def before_feed_update_hooks(self) -> MutableSequence[FeedUpdateHook]:
@@ -2247,7 +2231,7 @@ def before_feed_update_hooks(self) -> MutableSequence[FeedUpdateHook]:
             Wrap unexpected exceptions in :exc:`UpdateHookError`.
 
         """
-        return self._update_hooks['before_feed_update']
+        return self._update_hooks.hooks['before_feed_update']
 
     @property
     def after_entry_update_hooks(self) -> MutableSequence[AfterEntryUpdateHook]:
@@ -2284,7 +2268,7 @@ def after_entry_update_hooks(self) -> MutableSequence[AfterEntryUpdateHook]:
             Try to run all hooks, don't stop after one fails.
 
         """
-        return self._update_hooks['after_entry_update']
+        return self._update_hooks.hooks['after_entry_update']
 
     @property
     def after_feed_update_hooks(self) -> MutableSequence[FeedUpdateHook]:
@@ -2310,7 +2294,7 @@ def after_feed_update_hooks(self) -> MutableSequence[FeedUpdateHook]:
             Try to run all hooks, don't stop after one fails.
 
         """
-        return self._update_hooks['after_feed_update']
+        return self._update_hooks.hooks['after_feed_update']
 
     @property
     def after_feeds_update_hooks(self) -> MutableSequence[FeedsUpdateHook]:
@@ -2336,4 +2320,4 @@ def after_feeds_update_hooks(self) -> MutableSequence[FeedsUpdateHook]:
             Try to run all hooks, don't stop after one fails.
 
         """
-        return self._update_hooks['after_feeds_update']
+        return self._update_hooks.hooks['after_feeds_update']

src/reader/exceptions.py

@@ -4,12 +4,7 @@
 from collections.abc import Sequence
 from functools import cached_property
 from traceback import format_exception
-from typing import TYPE_CHECKING
-
-
-if TYPE_CHECKING:  # pragma: no cover
-    from ._types import UpdateHook
-    from ._types import UpdateHookType
+from typing import Any
 
 
 class _FancyExceptionBase(Exception):
@@ -323,8 +318,8 @@ class SingleUpdateHookError(UpdateHookError):
 
     def __init__(
         self,
-        when: UpdateHookType,
-        hook: UpdateHook,
+        when: str,
+        hook: Any,
         resource_id: tuple[str, ...] | None = None,
     ) -> None:
         super().__init__()