Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

ObservableDeferred's API is a mess #11390

Open
1 of 7 tasks
richvdh opened this issue Nov 18, 2021 · 0 comments
Open
1 of 7 tasks

ObservableDeferred's API is a mess #11390

richvdh opened this issue Nov 18, 2021 · 0 comments
Labels
P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@richvdh
Copy link
Member

richvdh commented Nov 18, 2021

... and I hate it.

In particular:

  • it doesn't consumeErrors by default, which in 99% of cases leads to log spam about unhandled failures
  • it does some awful __getattr__ / __setattr__ hijinks to "transparently" pass through the methods on the underlying Deferred, which makes it hard to type correctly and generally leads to magical behaviour.
  • I find it weird that you can fire an ObservableDeferred either by directly calling its callback/errback methods, or by calling them on the deferred you pass into it.

I would like to replace it with a much simpler implementation. Here is a proposal for how we can do it without rewriting everything at once.

  • Define an abstract base class which ObservableDeferred implements, called AbstractObservableDeferred, which just exposes the observe method:
    class AbstractObservableDeferred(Generic[T], metaclass=abc.ABCMeta):
        @abc.abstractmethod
        def observe(self) -> "Deferred[T]": ...
  • Replace consumers (rather than creators) of ObservableDeferred with AbstractObservableDeferred
  • Create a new class SimpleObservableDeferred which implementents AbstractObservableDeferred, and just has a callback, an errback, and an observe method. Both methods should return None.
  • Replace uses of ObservableDeferred which currently create a brand new Deferred with SimpleObservableDeferred
  • Create a new class DeferredObserver which wraps a SimpleObservableDeferred:
    class DeferredObserver(AbstractObservableDeferred[T]):
        def __init__(self): 
            self._observable = SimpleObservableDeferred[T]()
       def observe(self) -> Deferred[T]:
            return self._observable.observe()
       def chain_to_deferred(self, deferred: Deferred[T]) -> None:
            deferred.addCallbacks(self._observable.callback, self._observable.errback)
  • Replace uses of ObservableDeferred which chain to an existing Deferred with DeferredObserver
  • Remove now-redundant ObservableDeferred
@reivilibre reivilibre added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Dec 9, 2021
@callahad callahad added the P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches label Dec 9, 2021
reivilibre added a commit that referenced this issue Feb 10, 2022
reivilibre added a commit that referenced this issue Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

3 participants