Add plugin API for handling OutsideLifecycleExceptions. #57
Conversation
|
|
| /** | ||
| * Utility class to inject handlers to certain standard AutoDispose operations. | ||
| */ | ||
| public class AutoDisposePlugins { |
|
|
||
| /** | ||
| * Prevents changing the plugins from then on. | ||
| * <p>This allows container-like environments to prevent client messing with plugins.</p> |
There was a problem hiding this comment.
nit - nextline the text, no need for the closing paragraph marker
| ); | ||
| } | ||
|
|
||
| if (f != null) { |
There was a problem hiding this comment.
Turns out, this method isn't even used, I'm going to remove it.
| throw new LifecycleNotStartedException(); | ||
| LifecycleNotStartedException exception = new LifecycleNotStartedException(); | ||
| if (AutoDisposePlugins.outsideLifecycleHandler != null) { | ||
| AutoDisposePlugins.outsideLifecycleHandler.accept(exception); |
There was a problem hiding this comment.
Shouldn't this use the API rather than the fields directly?
There was a problem hiding this comment.
Also shouldn't we do something based if we proceed past accept? Like dispose or something?
| } | ||
| E endEvent; | ||
| try { | ||
| endEvent = provider.correspondingEvents() | ||
| .apply(lastEvent); | ||
| } catch (Exception e) { | ||
| if (checkEndBoundary && e instanceof LifecycleEndedException) { | ||
| throw e; | ||
| if (AutoDisposePlugins.outsideLifecycleHandler != null) { | ||
| AutoDisposePlugins.outsideLifecycleHandler.accept((LifecycleEndedException) e); |
There was a problem hiding this comment.
Same here with using the API
| throw e; | ||
| if (AutoDisposePlugins.outsideLifecycleHandler != null) { | ||
| AutoDisposePlugins.outsideLifecycleHandler.accept((LifecycleEndedException) e); | ||
| return Maybe.empty(); |
There was a problem hiding this comment.
Shouldn't this emit immediately to dispose immediately?
|
I think |
| throw e; | ||
| if (AutoDisposePlugins.outsideLifecycleHandler != null) { | ||
| AutoDisposePlugins.outsideLifecycleHandler.accept((LifecycleEndedException) e); | ||
| return Maybe.empty(); |
|
Wait I'm dumb. |
|
Ping on this |
| @@ -36,7 +36,8 @@ | |||
|
|
|||
| /** | |||
| * Prevents changing the plugins from then on. | |||
| * <p>This allows container-like environments to prevent client messing with plugins.</p> | |||
| * | |||
| .to(new FlowableScoper<Integer>(provider)) | ||
| .subscribe(o); | ||
|
|
||
| o.assertNotSubscribed(); |
There was a problem hiding this comment.
@hzsweers any idea why this would fail? This holds true for all the other types but for Flowable, it doesn't seem like it's the case.
There was a problem hiding this comment.
TestSubscriber and TestObserver are special/weird cases where they never unsubscribe. I'd just save the Disposable off and verify it's disposed.
|
I think the tests could be structured a bit differently. At the end of the day, the behavior is what we're interested in. The way that gets done might be an implementation detail, which this kind of verifies manually now (kind of like mockito but not quite). This is also not a conventional use case of What would you think of making it a more functional test? E.g.
Let me know what you think |
|
Also can you add a bit to the README? |
ZacSweers
changed the title
(Addresses #52)
The API is very similar to RxJavaPlugins.
One thing I'm a bit concerned about here - if you're providing your own
ScopeProviderorLifecycleScopeProvider, you need to either manually useScopeUtilor manually call the plugin yourself before throwing anything. It would be nicer if AutoDispose does all of this for you.