2.x: Implement as() #5729

ZacSweers · 2017-11-16T10:41:24Z

This implement as() support as discussed in #5654

I took the opportunity to try to standardize the docs and tests for it (which vary a little bit across implementations of to())

Related: #5654

ZacSweers · 2017-11-16T10:42:17Z

Let me know if you want to add more tests. Some usages of to() were mixed in other tests and wasn't sure if they were specifically targeting testing it.

ZacSweers · 2017-11-16T11:21:24Z

I'm not sure I understand the remaining CI errors, as the parameters have the NonNull annotation

akarnokd · 2017-11-16T12:01:50Z

src/main/java/io/reactivex/Completable.java

+    @Experimental
+    @CheckReturnValue
+    @SchedulerSupport(SchedulerSupport.NONE)
+    public final <R> R as(@NonNull CompletableConverter<? extends R> converter) {


Please add @Experimental and @since 2.1.7 - experimental to all of them.

akarnokd · 2017-11-16T12:02:11Z

src/main/java/io/reactivex/CompletableConverter.java

+ * value fluently.
+ *
+ * @param <R> the output type
+ */


Please add @since 2.1.7 - experimental to these as well.

akarnokd · 2017-11-16T12:07:52Z

Some style checks failed: https://travis-ci.org/ReactiveX/RxJava/builds/302962901#L974

Please add as to ParallelFlowable.

Please add a unit test that has a converter class which combines all interfaces and is applied to the 6 types of sources.

ZacSweers · 2017-11-16T12:10:01Z

Could you explain what the style checks fix is? I didn't quite understand the message in the log

ZacSweers · 2017-11-16T12:32:02Z

ParallelFlowable - 8adb583

Started on more converter tests in ConverterTest in c1f26ee, composite converter test is in there. Only thing I'm stuck on is the GenericsSignatureTest tests (borrowed from TransformersTest). Currently they don't compile, but I'm not sure what the goal is (or if it even makes sense to match the checks TransformersTest does). Let me know what you think

akarnokd · 2017-11-16T12:46:47Z

You have to extend the ParamValidationCheckerTest with instances of your new converter types. Put something like the following into L562.

defaultValues.put(ObservableConverter.class, new ObservableConverter() {
    @Override public Object apply(Observable o) { return o; }
});

akarnokd · 2017-11-16T13:24:06Z

As for these, you are using the wrong generic types.

~~ObservableConverter<A<T, ?>, B<T>> is applied to Observable<Integer> in the test where Integer is unrelated to A<T, ?>.~~

I overlooked the test using A. I'll check code locally.

akarnokd · 2017-11-16T14:05:55Z

The wildcard definition in those compile errors and the wrong <Integer> parameter confuses the compiler. I suggest going raw-types with these converters and using SuppressWarnings({"unchecked", "rawtypes"}) on the problematic tests.

akarnokd · 2017-11-16T14:23:47Z

Posted a patch for your PR.

as(): Fix tests and update validator

codecov · 2017-11-16T21:09:37Z

Codecov Report

Merging #5729 into 2.x will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #5729      +/-   ##
============================================
+ Coverage     96.24%   96.29%   +0.04%     
- Complexity     5823     5831       +8     
============================================
  Files           634      634              
  Lines         41609    41615       +6     
  Branches       5761     5761              
============================================
+ Hits          40047    40073      +26     
+ Misses          626      615      -11     
+ Partials        936      927       -9

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/reactivex/Observable.java	`100% <100%> (ø)`	`509 <1> (+1)`	⬆️
src/main/java/io/reactivex/Flowable.java	`100% <100%> (ø)`	`526 <1> (+1)`	⬆️
src/main/java/io/reactivex/Single.java	`100% <100%> (ø)`	`136 <1> (+1)`	⬆️
src/main/java/io/reactivex/Maybe.java	`100% <100%> (ø)`	`170 <1> (+1)`	⬆️
src/main/java/io/reactivex/Completable.java	`100% <100%> (ø)`	`114 <1> (+1)`	⬆️
...n/java/io/reactivex/parallel/ParallelFlowable.java	`100% <100%> (ø)`	`50 <1> (+1)`	⬆️
...in/java/io/reactivex/subjects/BehaviorSubject.java	`86.97% <0%> (-5.73%)`	`57% <0%> (ø)`
...l/operators/observable/ObservableFlatMapMaybe.java	`84.96% <0%> (-2.62%)`	`2% <0%> (ø)`
...activex/internal/observers/QueueDrainObserver.java	`61.53% <0%> (-2.57%)`	`12% <0%> (-1%)`
...ex/internal/subscribers/InnerQueuedSubscriber.java	`96.07% <0%> (-1.97%)`	`18% <0%> (-1%)`
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9521512...e0d793f. Read the comment docs.

akarnokd · 2017-11-16T21:11:11Z

src/main/java/io/reactivex/CompletableConverter.java

+     * @throws Exception on error
+     */
+    @NonNull
+    R apply(@NonNull Completable upstream) throws Exception;


The transformer variants don't define any throws Exception. I'd think we can live without them in these interfaces as well and thus no need to try-catch apply in the base classes' as() methods.

ZacSweers · 2017-11-17T01:36:25Z

Out of curiosity, would this be a candidate for @Beta in 2.2.0 since it's so close? Or would it stay @Experimental for a time period after?

akarnokd · 2017-11-17T09:26:38Z

2.2.0 is a bit in the future as I'd like to have #5319 finished before it. An experimental component can be promoted to stable regardless.

ZacSweers · 2017-11-17T11:09:10Z

Cool 👍

vanniktech

Should the copy right for the new files contain 2016 or 2017?

vanniktech · 2017-11-17T13:59:39Z

src/test/java/io/reactivex/ParamValidationCheckerTest.java

@@ -560,6 +560,46 @@ public void checkParallelFlowable() {

        defaultValues.put(ParallelFailureHandling.class, ParallelFailureHandling.ERROR);

+        @SuppressWarnings("rawtypes")
+        class MixedConverters implements FlowableConverter, ObservableConverter, SingleConverter,


Should we rename this to IdentityConverters?

I don't feel strongly about it since this is just a stub for a test

vanniktech · 2017-11-17T14:00:05Z

src/test/java/io/reactivex/completable/CompletableTest.java

+            }
+        });
+
+        flow.blockingForEach(new Consumer<Object>() {


this does not assertion is this wanted?

Deferring to @akarnokd on this as I was matching the analogous to() test

Please change these into proper assertions such as .test().assertResult().

What would I assert for these since there's no value? Or just rewrite the rest in general?

But there is the onComplete event.

there is but completable.normal doesn't complete. I wasn't sure if you wanted me to change it to just use Completable.complete() or something similar since the original test didn't seem to be testing completion

normal.completable does complete otherwise blockingForEach would block indefinitely and the test would have failed a long a go. A .test().assertResult(/* no values */) should suffice.

oops, I missed that line 🙄

Done in e0d793f

vanniktech · 2017-11-17T14:00:43Z

src/test/java/io/reactivex/flowable/FlowableTests.java

+            public Object apply(Flowable<Object> onSubscribe) {
+                    onSubscribe.subscribe(subscriber);
+                    subscriber.assertNoErrors();
+                    subscriber.assertComplete();


Could not this run through without asserting if the Converter is never called?

True... I could add something after this to assert that subscriber is complete?

How about an AtomicBoolean that you set to true once it's called and you fail the test when it's set to false

I think I got a better one - 5c3bdf3

vanniktech · 2017-11-17T14:00:56Z

src/test/java/io/reactivex/observable/ObservableTest.java

+        Observable.just(value).as(new ObservableConverter<Object, Object>() {
+            @Override
+            public Object apply(Observable<Object> onSubscribe) {
+                onSubscribe.subscribe(subscriber);


ZacSweers · 2017-11-19T11:10:06Z

Thanks!!

ZacSweers added 5 commits November 16, 2017 02:34

Implement Observable.as()

d7b5fc2

Implement Single.as()

eb276a6

Implement Maybe.as()

a847033

Implement Flowable.as()

d82c4ee

Implement Completable.as()

9bebf29

ZacSweers added 3 commits November 16, 2017 02:44

Add Experimental annotations

1aa5208

Add throws doc

68e2109

Fix docs and validation errors

8b67bbf

akarnokd reviewed Nov 16, 2017

View reviewed changes

Add @SInCE 2.1.7 - experimental

1914776

ZacSweers added 2 commits November 16, 2017 04:14

ParallelFlowable.as()

8adb583

Start ConverterTest

c1f26ee

akarnokd added 2.x Enhancement labels Nov 16, 2017

akarnokd added this to the 2.2 milestone Nov 16, 2017

Fix tests and update validator

e13a978

Merge pull request #2 from akarnokd/AsConverterFix

68a14b9

as(): Fix tests and update validator

akarnokd approved these changes Nov 16, 2017

View reviewed changes

akarnokd reviewed Nov 16, 2017

View reviewed changes

Remove exceptions from signatures

5c0dec5

Remove exception signature from implementations

c4b0c26

vanniktech reviewed Nov 17, 2017

View reviewed changes

ZacSweers and others added 2 commits November 17, 2017 17:30

Assert the full execution of extend() tests

5c3bdf3

Use test() helpers

e0d793f

akarnokd merged commit 6a44e5d into ReactiveX:2.x Nov 19, 2017

ZacSweers deleted the z/as branch November 19, 2017 11:10

ZacSweers mentioned this pull request Nov 19, 2017

3.x: Proposal: ObservableConverter interface and friends #5654

Closed

akarnokd mentioned this pull request Nov 22, 2017

2.1.7 release preparations #5739

Closed

ZacSweers mentioned this pull request Nov 27, 2017

Implement as()-based converter API uber/AutoDispose#140

Closed

jared-bot mentioned this pull request Dec 12, 2017

Dependency update: rxjava xmartlabs/bigbang-template#33

Closed

jared-bot mentioned this pull request Dec 27, 2017

Dependency update: rxjava xmartlabs/bigbang-template#45

Merged

akarnokd mentioned this pull request Nov 21, 2018

What's the purpose of having to, as, and compose? #6319

Closed

2.x: Implement as() #5729

2.x: Implement as() #5729

Conversation

ZacSweers commented Nov 16, 2017 • edited by akarnokd Loading

ZacSweers commented Nov 16, 2017

ZacSweers commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akarnokd commented Nov 16, 2017

ZacSweers commented Nov 16, 2017

ZacSweers commented Nov 16, 2017

akarnokd commented Nov 16, 2017

akarnokd commented Nov 16, 2017 • edited Loading

akarnokd commented Nov 16, 2017

akarnokd commented Nov 16, 2017

codecov bot commented Nov 16, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Nov 17, 2017 • edited Loading

akarnokd commented Nov 17, 2017

ZacSweers commented Nov 17, 2017

vanniktech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacSweers commented Nov 19, 2017

ZacSweers commented Nov 16, 2017 •

edited by akarnokd

Loading

akarnokd commented Nov 16, 2017 •

edited

Loading

codecov bot commented Nov 16, 2017 •

edited

Loading

ZacSweers commented Nov 17, 2017 •

edited

Loading

ZacSweers Nov 19, 2017 •

edited

Loading