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.

Sign up for GitHub

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

Jump to bottom

RxLifecycle interop module #118

Merged

ZacSweers merged 16 commits into uber:master from bangarharshit:autodispose-rxlifecycle-interop

Oct 22, 2017

Contributor

bangarharshit commented Oct 21, 2017 •

edited by ZacSweers

Loading

RxLifecycle interop module added.

Resolves #82

CLAassistant commented Oct 21, 2017 •

edited

Loading

All committers have signed the CLA.

Contributor Author

bangarharshit commented Oct 21, 2017

Looks like the build timed out. Looking into the log

ZacSweers requested changes

View reviewed changes

Collaborator

ZacSweers left a comment

Good start. Just a note for something I noticed in a few places. It should be spelled RxLifecycle, not RXLifeCycle. Please update that in any places I didn't comment on as well

...cle-interop/src/main/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInterop.java Outdated


		private static final Object DEFAULT_THROWAWAY_OBJECT = new Object();

		static <E> ScopeProvider bindLifecycle(final LifecycleProvider<E> provider) {

Collaborator

ZacSweers Oct 21, 2017

These methods should both be public

Collaborator

ZacSweers Oct 21, 2017

On further thinking, I think we should call both of these methods from for consistency with the rest of AutoDispose

...cle-interop/src/main/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInterop.java Outdated

+               * as normal terminal event. There is no mapping to {@link LifecycleEndedException} and in such
+               * cases the stream is normally disposed </em>
+               */
+              public final class RXLifeCycleInterop {

Collaborator

ZacSweers Oct 21, 2017

This should be called RxLifecycleInterop

...cle-interop/src/main/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInterop.java Outdated

+              /**
+               * Interop class for {@link RxLifecycle}. It provides static utility methods to convert {@link
+               * LifecycleProvider} to {@link ScopeProvider}
+               * <p> There are several static utility converter

Collaborator

ZacSweers Oct 21, 2017

Nit: new line for the <p>, remove the space after

Contributor Author

bangarharshit Oct 21, 2017

Quick check - What is the code style? My formatter did it automatically.

Collaborator

ZacSweers Oct 21, 2017

code style is basically the square code style - https://github.com/square/java-code-styles

...cle-interop/src/main/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInterop.java Outdated

+              import io.reactivex.Maybe;
+              /**
+               * Interop class for {@link RxLifecycle}. It provides static utility methods to convert {@link

Collaborator

ZacSweers Oct 21, 2017

Nit: don't link RxLifecycle since you're talking about the library and not the class. Same with the other link below

...cle-interop/src/main/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInterop.java Outdated

+               * LifecycleProvider#bindToLifecycle()} and {@link #bindUntilEvent(LifecycleProvider, Object)} for
+               * {@link LifecycleProvider#bindUntilEvent(Object)} </p>
+               *
+               * <em>Unlike {@link AutoDispose}, {@link RxLifecycle} treats the {@link OutsideLifecycleException}

Collaborator

ZacSweers Oct 21, 2017

Nit: remove the autodispose bit at the beginning

also, only make the beginning emphasized. Do something like:

<em>Note:</em> RxLifecycle treats...

...interop/src/test/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInteropTest.java Outdated


		import static com.google.common.truth.Truth.assertThat;

		public class RXLifeCycleInteropTest {

Collaborator

ZacSweers Oct 21, 2017

RxLifecycleInteropTest

...interop/src/test/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInteropTest.java Outdated

+                  }
+                };
+                private LifeCycleProviderImpl lifeCycleProvider = new LifeCycleProviderImpl();

Collaborator

ZacSweers Oct 21, 2017

lifecycleProvider

...interop/src/test/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInteropTest.java Outdated

+                @Before
+                public void setup() {
+                  lifeCycleProvider.onCreate();

Collaborator

ZacSweers Oct 21, 2017

Let's mark this explicitly in each test

settings.gradle Outdated

@@ @@ -25,3 +25,5 @@ include ':autodispose' @@
               include ':autodispose-kotlin'
               include ':sample'
               include ':test-utils'
+              include 'autodispose-rxlifecycle-interop'

Collaborator

ZacSweers Oct 21, 2017

Let's call this just autodispose-rxlifecycle, and make the package name com.uber.autodispose.rxlifecycle.

...interop/src/test/java/com/ubercab/autodispose/rxlifecycleinterop/RXLifeCycleInteropTest.java Outdated

+                }
+                @Test
+                public void bindLifecycle_errorTermination_unsubscribe() {

Collaborator

ZacSweers Oct 21, 2017

Should the method name say something about subscribing outside of lifecycle bounds?

ZacSweers reviewed

View reviewed changes

autodispose-rxlifecycle-interop/build.gradle Outdated

@@ @@ -25,7 +25,7 @@ dependencies { @@
                   testApt deps.build.nullAway
                   compile project(':autodispose')
-                  compile 'com.trello.rxlifecycle2:rxlifecycle:2.2.0'
+                  api deps.rx.lifecycle

Collaborator

ZacSweers Oct 21, 2017

this should be compile as we're still on AGP 2.3.3

autodispose-rxlifecycle-interop/gradle.properties Outdated

		@@ -0,0 +1,3 @@
		POM_NAME=AutoDispose (RXLifeCycle Interop)

Collaborator

ZacSweers Oct 21, 2017

RxLifecycle

autodispose-rxlifecycle-interop/gradle.properties Outdated

		@@ -0,0 +1,3 @@
		POM_NAME=AutoDispose (RXLifeCycle Interop)
		POM_ARTIFACT_ID=autodispose-rxlifecycle-interop

Collaborator

ZacSweers Oct 21, 2017

autodispose-rxlifecycle

gradle/dependencies.gradle Outdated

@@ @@ -57,7 +57,8 @@ def misc = [ @@
               def rx = [
                   android: 'io.reactivex.rxjava2:rxandroid:2.0.1',
-                  java: 'io.reactivex.rxjava2:rxjava:2.1.0'
+                  java: 'io.reactivex.rxjava2:rxjava:2.1.0',
+                  lifecycle: 'com.trello.rxlifecycle2:rxlifecycle:2.2.0'

Collaborator

ZacSweers Oct 21, 2017

this should be separate, this block is just for reactivex. Probably under misc

Contributor Author

bangarharshit Oct 21, 2017

Fixed top-level comments - renaming and dependencies.gradle in cbf3e10

ZacSweers changed the title ~~RX lifecycle interop module added for autodipose~~ RxLifecycle interop module

Collaborator

ZacSweers commented Oct 21, 2017

FYI for future reference, it's much easier for the reviewer if you address each comment (± group of comments if they go together) in individual commits and link them in a reply, that way it's easy to connect the changes :)

Contributor Author

bangarharshit commented Oct 21, 2017

Will take care of it from future.

ZacSweers requested changes

View reviewed changes

Collaborator

ZacSweers left a comment

Almost there!

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RXLifecycleInterop.java Outdated

-               * methods such as {@link #bindLifecycle(LifecycleProvider)} for {@link
-               * LifecycleProvider#bindToLifecycle()} and {@link #bindUntilEvent(LifecycleProvider, Object)} for
-               * {@link LifecycleProvider#bindUntilEvent(Object)} </p>
+               * Interop class for RxLifecycle. It provides static utility methods to convert {@link

Collaborator

ZacSweers Oct 21, 2017

nit: remove class

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RXLifecycleInterop.java Outdated

    
                  throw new AssertionError("No Instances");

                }

                private static final Object DEFAULT_THROWAWAY_OBJECT = new Object();

                static <E> ScopeProvider bindLifecycle(final LifecycleProvider<E> provider) {

                public static <E> ScopeProvider fromBindLifecycle(final LifecycleProvider<E> provider) {

Collaborator

ZacSweers Oct 21, 2017

I meant just call them from, not fromBindLifecycle or anything

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RXLifecycleInterop.java Outdated

                */
-              public final class RXLifeCycleInterop {
+              public final class RXLifecycleInterop {

Collaborator

ZacSweers Oct 21, 2017

Rx, not RX

...se-rxlifecycle/src/test/java/com/ubercab/autodispose/rxlifecycle/RXLifecycleInteropTest.java Outdated

               import org.junit.Test;
               import static com.google.common.truth.Truth.assertThat;
-              public class RXLifeCycleInteropTest {
+              public class RXLifecycleInteropTest {

Collaborator

ZacSweers Oct 21, 2017

Rx, not RX

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RXLifecycleInterop.java Outdated

+               * methods such as {@link #fromBindLifecycle(LifecycleProvider)} for {@link
+               * LifecycleProvider#bindToLifecycle()} and {@link #fromBindUntilEvent(LifecycleProvider, Object)} for
+               * {@link LifecycleProvider#bindUntilEvent(Object)}.
+               * </p>

Collaborator

ZacSweers Oct 21, 2017

Remove this, add a <p> below

Contributor Author

bangarharshit Oct 21, 2017

Fixed - 7c73cc2. Code review in github can be a pain.

Collaborator

ZacSweers Oct 21, 2017

Much better than phabricator though! :)

ZacSweers reviewed

View reviewed changes

autodispose-rxlifecycle/gradle.properties Outdated

		@@ -1,3 +1,3 @@
		POM_NAME=AutoDispose (RXLifeCycle Interop)
		POM_NAME=AutoDispose (RXLifecycle Interop)

Collaborator

ZacSweers Oct 21, 2017

RxLifecycle

...se-rxlifecycle/src/test/java/com/ubercab/autodispose/rxlifecycle/RXLifecycleInteropTest.java Outdated

                   source.onNext(2);
                   o.assertNoMoreEvents();
                   assertThat(
-                      source.hasObservers()).isFalse(); // Because RXLifeCycle treats lifecycle ended exception as terminal event.
+                      source.hasObservers()).isFalse(); // Because RXLifecycle treats lifecycle ended exception as terminal event.

Collaborator

ZacSweers Oct 21, 2017

RxLifecycle

gradle/dependencies.gradle Outdated

@@ @@ -52,13 +52,13 @@ def kotlin = [ @@
               def misc = [
                   errorProneAnnotations: "com.google.errorprone:error_prone_annotations:${versions.errorProne}",
                   javaxExtras: "com.uber.javaxextras:javax-extras:0.1.0",
-                  jsr305: 'com.google.code.findbugs:jsr305:3.0.2'
+                  jsr305: 'com.google.code.findbugs:jsr305:3.0.2',
+                  lifecycle: 'com.trello.rxlifecycle2:rxlifecycle:2.2.0'

Collaborator

ZacSweers Oct 21, 2017

rxlifecycle

ZacSweers requested changes

View reviewed changes

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RxLifecycleInterop.java Outdated

                   throw new AssertionError("No Instances");
                 }
                 private static final Object DEFAULT_THROWAWAY_OBJECT = new Object();
-                public static <E> ScopeProvider fromBindLifecycle(final LifecycleProvider<E> provider) {
+                public static <E> ScopeProvider fromLifecycle(final LifecycleProvider<E> provider) {

Collaborator

ZacSweers Oct 21, 2017

from(...). Same with the one below

ZacSweers reviewed

View reviewed changes

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RxLifecycleInterop.java Outdated

@@ @@ -28,7 +28,7 @@ private RxLifecycleInterop() { @@
                 private static final Object DEFAULT_THROWAWAY_OBJECT = new Object();
-                public static <E> ScopeProvider fromLifecycle(final LifecycleProvider<E> provider) {
+                public static <E> ScopeProvider from(final LifecycleProvider<E> provider) {

Collaborator

ZacSweers Oct 21, 2017

Missed this in my earlier comments, but these two methods should each have docs

Contributor Author

bangarharshit Oct 21, 2017

Fixed in c2e63ff

ZacSweers reviewed

View reviewed changes

...ispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/RxLifecycleInterop.java Outdated

@@ @@ -40,6 +56,22 @@ private RxLifecycleInterop() { @@
                   };
                 }
+                /**
+                 * Converter for  transforming {@link LifecycleProvider} to {@link ScopeProvider}.

Collaborator

ZacSweers Oct 21, 2017

Nit: fix the double space after for in this and the one above

Contributor Author

bangarharshit Oct 21, 2017

Fixed - b373d42

Contributor Author

bangarharshit commented Oct 21, 2017 •

edited

Loading

For some weird reason - build is getting a timeout at accept license. - raw log. I am not making any change in the .travis.yml file.

Master build on my fork is also facing the same issue.

Collaborator

ZacSweers commented Oct 21, 2017 •

edited

Loading

Licenses in the sdk seem to have changed. Can you try changing the travis yml file with the following patch?

Index: .travis.yml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- .travis.yml	(revision 32e5c564a2da708566d7ba876eb86d84247789a5)
+++ .travis.yml	(revision )
@@ -12,7 +12,8 @@
 before_install:
   # Install SDK license so Android Gradle plugin can install deps.
   - mkdir "$ANDROID_HOME/licenses" || true
-  - echo "$LICENSES_SHA" > "$ANDROID_HOME/licenses/android-sdk-license"
+  - echo "$LICENSES_HASH" > "$ANDROID_HOME/licenses/android-sdk-license"
+  - echo "$LICENSES_HASH_TWO" >> "$ANDROID_HOME/licenses/android-sdk-license"
   # Install the rest of tools (e.g., avdmanager)
   - sdkmanager tools
   # Install the system image

ZacSweers requested changes

View reviewed changes

autodispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/package-info.java Outdated

+               */
+              /**
+               * Interop for RxLifecycle.

Collaborator

ZacSweers Oct 21, 2017

Please write a more detailed doc, this is the doc for the entire package

Contributor Author

bangarharshit Oct 22, 2017

Added more details and license - e463be6

Collaborator

ZacSweers commented Oct 21, 2017

Confirmed the above patch fixes it in #119. If you want to just include that in your PR that should fix it, and I'll close out the other one

Collaborator

ZacSweers commented Oct 21, 2017

Merged that other one, just rebase your changes here

Contributor Author

bangarharshit commented Oct 22, 2017

Thanks. One quick question - do I need to copy license in each file or is it taken care of while pushing?

Collaborator

ZacSweers commented Oct 22, 2017

Each file will need a license header yeah. You can install a profile into intelliJ to automatically add them

bangarharshit added 11 commits

October 22, 2017 05:44


          RX lifecycle interop module added for autodipose

d078017


          Added pom artifact

159205a


          Renamed package, fixed javadoc and addressed other review comments

25f3868


          Fixed lifecycle everywhere and moved rxlifecycle to misc in dependenc…

19a0445

…ies.gradle


          Fixed javadoc

12840a9


          Renamed RXLifecycle to RxLifecycle and method name fix

335f22b


          Renamed depdencies in dependency.gradle and removed class from javadoc

5f2be30


          Renamed methods in RXLifecycle to from() for consistency

11f6174


          Added javadoc for individual methods

ce14957


          Removed double space

c38dd9c


          Fixed checkstyle for test

f33e1c7

bangarharshit added 4 commits

October 22, 2017 05:44


          fixed exception name

06be7fb


          API replaced with implementation to avoid transitive compile time dep…

51d7989

…endencies


          Reverted the incorrect api

7182d54


          Added more javadoc in package-info.java and license header to all the…

e463be6

… files

ZacSweers reviewed

View reviewed changes

autodispose-rxlifecycle/src/main/java/com/ubercab/autodispose/rxlifecycle/package-info.java Outdated

@@ @@ -15,7 +15,9 @@ @@
                */
               /**
-               * Interop for RxLifecycle.
+               * Interop for RxLifecycle. It could understand RxLifecycle's

Collaborator

ZacSweers Oct 22, 2017

I think we can phrase this better, doesn't have to be too wordy. Let's do

AutoDispose extensions for interop with RxLifecycle. This namely supports {@link LifecycleProvider}.

Contributor Author

bangarharshit Oct 22, 2017

Fixed - 7f3e1a8


          Fixed javadoc

7f3e1a8

ZacSweers approved these changes

View reviewed changes

Collaborator

ZacSweers left a comment

Nice work! Pending CI results now :)

ZacSweers merged commit 60d95bf into uber:master

Collaborator

ZacSweers commented Oct 22, 2017

Thanks Harshit!

ZacSweers mentioned this pull request

Release notes for 0.4.0 #115

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet