-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
1.x: Change Completable.subscribe(onError, onComplete) to (onComplete, onError) #4140
1.x: Change Completable.subscribe(onError, onComplete) to (onComplete, onError) #4140
Conversation
}); | ||
} | ||
|
||
@Test(expected = NullPointerException.class) | ||
public void subscribeTwoCallbacksSecondNull() { | ||
normal.completable.subscribe(null, new Action0() { |
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.
Didn't catch that on initial PR, test was obviously copied from subscribeTwoCallbacksFirstNull
and wasn't changed, second parameter should have been null
.
8912d87
to
55723da
Compare
Actually, I can deprecate old one and add new one as overload, it will have less impact on user code because only calls like Thoughts? |
Current coverage is 81.33%@@ 1.x #4140 diff @@
==========================================
Files 257 257
Lines 16811 16811
Methods 0 0
Messages 0 0
Branches 2547 2547
==========================================
+ Hits 13666 13673 +7
+ Misses 2243 2238 -5
+ Partials 902 900 -2
|
👍 |
Dynamic languages have trouble with the two methods, let's only have 1 method. |
👍 |
Closes #3851, closes #4137.