Add support for exposing delegate observers.#89
Conversation
ZacSweers
left a comment
There was a problem hiding this comment.
Looking good! From a high level I think there's a few changes we should make that are across the observers:
- Let's inline the
{@link Observer}mentions in docs, it should read like a sentence since they are linkified inline. Make sure you link the correct observer as well (e.g. right now they all useObserverbut there is a uniqueObserverfor each type, likeSingleObserver). - Let's add a comment to the docs that explicitly call out that this API will change once the LambdaIntrospection stuff is out of
@Experimentalin RxJava (at which point we'll update it to return that interface instead and mark it nullable to return null upon non conforming types). - Let's add
@Experimentalto all the methods to indicate their plans to change. Borrow the RxJava annotation. - Let's make the atomic references in tests local to their specific unit tests rather than fields since they're only used in those methods.
- Use reference comparison for the delegate observer in tests, not
isEqualTo(which checks equality and isn't what we want).
| * for type safety but enforcement is left to the implementation. | ||
| */ | ||
| public interface AutoDisposingSubscriber<T> extends Subscriber<T>, Subscription, Disposable {} | ||
| public interface AutoDisposingSubscriber<T> extends FlowableSubscriber<T>, Subscription, Disposable {} |
There was a problem hiding this comment.
Nit: would be good to include a tidbit of the explanation you put in the PR description in this commit message
| public interface AutoDisposingObserver<T> extends Observer<T>, Disposable { | ||
|
|
||
| /** | ||
| * @return {@link Observer} The delegate Observer that is used under the hood for introspection purposes. |
There was a problem hiding this comment.
Nit: Did you mean to link the Observer here rather than inline?
I'd expect this to be like @return The delegate {@link Observer} that is used under the hood for introspection purposes.
| assertThat(atomicAutoDisposingObserver.get()).isInstanceOf(AutoDisposingObserver.class); | ||
| assertThat(((AutoDisposingObserver)atomicAutoDisposingObserver.get()).delegateObserver()).isNotNull(); | ||
| assertThat(((AutoDisposingObserver)atomicAutoDisposingObserver.get()).delegateObserver()) | ||
| .isEqualTo(atomicObserver.get()); |
There was a problem hiding this comment.
This should be a reference check, not equality
|
|
||
| public class AutoDisposeObserverTest { | ||
|
|
||
| private final AtomicReference<Observer> atomicObserver = new AtomicReference(); |
There was a problem hiding this comment.
since these are only used in one test, let's make them final local variables in those methods instead
| source.onNext(2); | ||
|
|
||
| assertThat(source.hasObservers()).isTrue(); | ||
| assertThat(source.hasObservers()).isTrue();Å |
There was a problem hiding this comment.
I believe this was from you trying to use my keyboard.
|
Resolves #78
This change updated
AutoDisposingSubscriberimplementFlowableSubscriberinstead ofSubscriber. We wanted to have more consistency with how the rest of theObservers are handled in RxJava. This change remains backward compatible becauseFlowableSubscriberstill extendsSubscriber.This also exposes all the different implementations of
Observerto allow for introspection to the delegateObserver. This will be particularly useful for ReactiveX/RxJava#5590.