-
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
Structured Android components (and some refactorings) #111
Conversation
This renames TestAndroidLifecycleScopeProvider to the more appropriate TestLifecycleOwner. Amongst other things, this makes the extensions/common dependencies only part of that artifact and not a part of the mainline artifact
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.
lgtm
I'll do a pass to update license headers, seems my intelliJ lost the profile :|. Test failure should be easy to fix too |
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.
lgtm
gradle/dependencies.gradle
Outdated
@@ -66,7 +66,8 @@ def support = [ | |||
lifecycle: [ | |||
common: "android.arch.lifecycle:common:${versions.archComponents}", | |||
compiler: "android.arch.lifecycle:compiler:${versions.archComponents}", | |||
extensions: "android.arch.lifecycle:extensions:${versions.archComponents}" | |||
extensions: "android.arch.lifecycle:extensions:${versions.archComponents}", | |||
runtime: "android.arch.lifecycle:runtime:1.0.0" |
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.
Do runtime have different release cycle, so you don't use the same version?
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.
Yeah runtime is already stable and released
include ':android:autodispose-android-archcomponents' | ||
include ':android:autodispose-android-archcomponents-kotlin' | ||
include ':android:autodispose-android-archcomponents-test' | ||
include ':android:autodispose-android-archcomponents-test-kotlin' |
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.
Well, I have some concerns here. Looks like we have few dimensions (like lifecycle events provider, platform, language), and implementing or updating new thing for any of it leads to rewriting of N sublibraries. Maybe you have an idea how to resolve it, or we can discuss offline and try to find a solution.
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.
What do you mean by rewriting?
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.
Like, imagine, you want to update autodispose-kotlin. It'll (possibly) lead to updates in autodispose-android-archcomponents-kotlin and autodispose-android-archcomponents-test-kotlin (actually, any artifact mentions kotlin).The same condition will stay for, say, update in archcomponents. If we have it somewhat decoupled (I don't have solution for this, it's only an brainstorm-ish idea!), then we can leverage modularity better. Also, it'll be better for library consumers to have more granular control over dependencies.
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.
Ah, no the plan is for them to always be in sync even if some artifacts have no changes
I can't make heads or tails of this error. All the android artifacts use different, unique package names 🤔 |
This is apparently necessary for generated code
This also simplifies the multiple constructors we had going on
Due to a behavior change, stop events just mean it falls back to "created", which presumably could restart. To match this, I've changed the test to measure this such that if the last even was pause, we don't assume destroy, and actually just stop in onStop like normal
This is a somewhat big PR, but isolated commits
Overview:
android/
TestAndroidLifecycleScopeProvider
to its ownautodispose-android-archcomponents-test
artifact, rename to more appropriateTestLifecycleOwner
.Resolves #87
Resolves #112
Resolves #113