Skip to content
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: Remove 'Completable' prefix from nested interfaces, move its subscription to top-level. #4425

Merged
merged 1 commit into from
Aug 30, 2016
Merged

1.x: Remove 'Completable' prefix from nested interfaces, move its subscription to top-level. #4425

merged 1 commit into from
Aug 30, 2016

Conversation

JakeWharton
Copy link
Contributor

This is only the Completable part of #4420. It does not make CompletableSubscriber an abstract class nor create Single.Operator.

@akarnokd
Copy link
Member

Compiler error.

@codecov-io
Copy link

codecov-io commented Aug 25, 2016

Current coverage is 84.23% (diff: 68.04%)

Merging #4425 into 1.x will decrease coverage by 0.16%

@@                1.x      #4425   diff @@
==========================================
  Files           270        270          
  Lines         17518      17548    +30   
  Methods           0          0          
  Messages          0          0          
  Branches       2677       2677          
==========================================
- Hits          14785      14781     -4   
- Misses         1877       1909    +32   
- Partials        856        858     +2   

Powered by Codecov. Last update 743f164...55bf5e7

@akarnokd
Copy link
Member

Looks okay. What about the @Deprecated markers?

@JakeWharton
Copy link
Contributor Author

Oh, sure. I'll add them in real quick. I wasn't sure on your stance on them, but I think it'll make migration easier.

@akarnokd
Copy link
Member

Yes, having deprecated markers is more forgiveable than lacking the entire type. I don't know how extensively Completable is in use by other libraries right now (beyond RxJavaReactiveStreams, Reactor's converter, Retrofit`).

@JakeWharton
Copy link
Contributor Author

Updated!


/**
* Convenience interface and callback used by the lift operator that given a child CompletableSubscriber,
* return a parent CompletableSubscriber that does any kind of lifecycle-related transformations.
*/
public interface CompletableOperator extends Func1<CompletableSubscriber, CompletableSubscriber> {
public interface Operator extends Func1<rx.CompletableSubscriber, rx.CompletableSubscriber> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain about this being a compatible change.

So does this still compile?

CompletableOperator op = new CompletableOperator() {
    @Override
    public Completable.CompletableSubscriber call(Completable.CompletableSubscriber cs) {
         return cs;
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah it's not possible to provide perfect migration using this strategy. The covariant return type works but not the argument. We could provide overloads of lift, compose, and subscribe which accepted the old types, adapted them to the new type, and were marked as deprecated. That should provide perfect compatibility.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, how about the old ones not extending the new ones and have wrappers inside those methods of the old types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with overloads

@akarnokd
Copy link
Member

@JakeWharton
Copy link
Contributor Author

Compilation issue resolved.

@akarnokd
Copy link
Member

I know, still waiting for a second approval.

@akarnokd akarnokd merged commit db3ff46 into ReactiveX:1.x Aug 30, 2016
@akarnokd
Copy link
Member

Okay, let's have this. One can complain anytime later.

@JakeWharton JakeWharton deleted the jw/start-normalization branch October 24, 2016 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants