1.x: Added Single execution hooks #3696
Conversation
|
Would you also apply these plugin methods in Single?
|
|
Below hooks are already called (I replaced previous calls of
However there is problem with |
|
There is something wrong, a bunch of Single test failed. |
|
👍 |
| @@ -1569,14 +1569,12 @@ public final void onNext(T args) { | |||
| * @param subscriber | |||
| * the Subscriber that will handle the emission or notification from the Single | |||
| */ | |||
| public final void unsafeSubscribe(Subscriber<? super T> subscriber) { | |||
| public final Subscription unsafeSubscribe(Subscriber<? super T> subscriber) { | |||
There was a problem hiding this comment.
Looks like this is incompatible binary change https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.15
There was a problem hiding this comment.
I strive to be as much as possible compatible with Observable type. Do you have some proposition how to handle it? If we remove Subscription as return type then we won't be able to decorate or replace the Subscription instance in hook.
There was a problem hiding this comment.
Right right, I mean that we will have to release it as 1.2 instead of 1.1.x, I guess.
Maintainers: what do you think?
There was a problem hiding this comment.
Unsafesubscribe is called by operators and they don't use the returned Subscription. These unsubscribe the Subscriber.
There was a problem hiding this comment.
Single isn't stable API so this is acceptable if it makes the API better.
On Fri, Feb 19, 2016, 10:46 AM Artem Zinnatullin [email protected]
wrote:
In src/main/java/rx/Single.java
#3696 (comment):@@ -1569,14 +1569,12 @@ public final void onNext(T args) {
* @param subscriber
* the Subscriber that will handle the emission or notification from the Single
*/
- public final void unsafeSubscribe(Subscriber<? super T> subscriber) {
- public final Subscription unsafeSubscribe(Subscriber<? super T> subscriber) {
Looks like this is incompatible binary change
https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.15—
Reply to this email directly or view it on GitHub
https://github.com/ReactiveX/RxJava/pull/3696/files#r53476625.
There was a problem hiding this comment.
k, let's just mention this as kind of warning in changelog
There was a problem hiding this comment.
Yes this is a breaking binary change, but thanks to the fact that Single is not yet marked as stable, I vote for making this change.
|
As far I see that failing test is not related with my changes. Is |
| @@ -100,7 +100,7 @@ private Single(final Observable.OnSubscribe<T> f) { | |||
| this.onSubscribe = f; | |||
| } | |||
|
|
|||
| static final RxJavaObservableExecutionHook hook = RxJavaPlugins.getInstance().getObservableExecutionHook(); | |||
| static RxJavaSingleExecutionHook hook = RxJavaPlugins.getInstance().getSingleExecutionHook(); | |||
There was a problem hiding this comment.
Nevermind, I just saw your last comment.
|
More tests would be appreciated, but otherwise LGTM. |
|
Could you squash the commits and rebase this PR? |
|
👍 (after you rebase/squash the commits) |
phajduk
force-pushed
the
SingleHooks
branch
2 times, most recently
from
76710a3 to
7a446a4
Compare
1.x: Enabled Single onSubscribeStart hook 1.x: Added more Single hooks unit tests
|
👍 |
1.x: Added Single execution hooks
Resolves #3595