-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract LifecycleScopeProvider to separate artifact #228
Conversation
This is where OutsideScope exceptions can be thrown
import org.junit.Test; | ||
|
||
import static com.google.common.truth.Truth.assertThat; | ||
|
||
public final class AutoDisposeAndroidPluginsTest { | ||
|
||
@After public void tearDown() { | ||
@Before @After public void tearDown() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tearDown is probably a bad name if you're using this for before and after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of Part of 53772ce
@@ -17,7 +33,7 @@ | |||
@Documented | |||
@Retention(RetentionPolicy.CLASS) | |||
@Target(ElementType.TYPE) | |||
@interface DoNotMock { | |||
public @interface DoNotMock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid making this public, will show up on other folks classpaths and may be somewhat common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline, not really avoidable unfortunately unless we wanted to make it a separate compileOnly artifact
import org.junit.Test; | ||
|
||
import static com.google.common.truth.Truth.assertThat; | ||
|
||
public final class AutoDisposePluginsTest { | ||
|
||
@After public void tearDown() { | ||
@Before @After public void tearDown() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as other tearDown comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of 53772ce
*/ | ||
@CheckReturnValue | ||
inline fun <T> Flowable<T>.autoDisposable( | ||
provider: com.uber.autodispose.lifecycle.LifecycleScopeProvider<*>): FlowableSubscribeProxy<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduce to class name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of 7fcd5a1
.autoDisposable(lifecycleScopeProvider) | ||
.subscribe(o) | ||
|
||
o.assertError { it is com.uber.autodispose.lifecycle.LifecycleNotStartedException } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of 7fcd5a1
lifecycleScopeProvider.stop() | ||
|
||
// https://github.com/ReactiveX/RxJava/issues/5178 | ||
// assertThat(o.isDisposed).isTrue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We leave these to indicate what we would assert/expect there but can't due to the linked issue
} | ||
|
||
tasks.withType(JavaCompile) { | ||
options.compilerArgs += ["-Xep:NullAway:ERROR", "-XepOpt:NullAway:AnnotatedPackages=com.uber"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract string to dedup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside the scope of this PR
|
||
public final class LifecycleScopesTest { | ||
|
||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo for this diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, no big concerns.
This simplifies the API a bit while also tightening the exception possibilities. We now use a null peekEvent() to determine it to be a non-started lifecycle
Resolves #197
This extracts
LifecycleScopeProvider
and its friends to a separate artifact and makes it extendScopeProvider
.It's a bit long, but each commit is tiny and easy to digest.
Changes worth documenting:
LifecycleScopeProvider
is now in a separateautodispose-lifecycle
artifact and extendsScopeProvider
. The package name iscom.uber.autodispose.lifecycle
.AutoDisposePlugins
has moved toAutoDisposeLifecyclePlugins
since all the configuration was for lifecycle behavior. We can always bringAutoDisposePlugins
back to the main artifact separately in the future.OutsideLifecycleException
is nowOutsideScopeException
, and was not moved to the new lifecycle artifact.LifecycleEndNotification
is no longer publicScopeUtil
's lifecycle-related methods are now moved toLifecycleScopes
TestLifecycleScopeProvider
is moved to the new artifactautodispose-lifecycle-jdk8
artifact with aDefaultLifecycleScopeProvider
that stubs adefault
implementation forrequestScope()
autodispose-lifecycle-ktx
artifact with extension functions forLifecycleScopeProvider
. This-ktx
name will be applied to the other artifacts as well (planned before 1.0).LifecycleScopeProvider
changes:correspondingEvents
now returns a more specificFunction
calledCorrespondingEventsFunction
, which can only throw anOutsideScopeException
and simplifies the generics@Nullable
rather than Jetbrains