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.

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

Disambiguous some method call sites when calling from Kotlin #6551

Closed
vanniktech opened this issue Jun 30, 2019 · 7 comments · Fixed by #6556
Closed

Disambiguous some method call sites when calling from Kotlin #6551

vanniktech opened this issue Jun 30, 2019 · 7 comments · Fixed by #6556
Labels
3.x PR welcome
Milestone
3.0

Comments

@vanniktech
Copy link
Collaborator

I vaguely remember that we already had a discussion about this but I don't know which conclusion was drawn. Observable.onErrorResumeNext is ambiguous when calling from Kotlin.

Screenshot 2019-06-30 at 12 09 33

Since 3.x is a thing do we want to rename these methods. Similar to what we did with startWith (even though there was no type inference problem).
@akarnokd
Copy link
Member

Yes, 3.x is a good place fix such ambiguities. My ideas are onErrorResume(ObservableSource) or onErrorResumeWith(ObservableSource). PR welcome.

@akarnokd akarnokd added this to the 3.0 milestone Jun 30, 2019
@vanniktech
Copy link
Collaborator Author

Those names sound good to me. I believe the onErrorReturn should also be renamd then.

@luis-cortes
Copy link
Contributor

luis-cortes commented Jul 3, 2019 •
edited
Loading

A few questions:

  • Do you want to change Maybe.onErrorResumeNext(MaybeSource) as well?
  • To what should Observable.onErrorReturn(Function valueSupplier) be renamed?
  • Do you want to rename Maybe.onErrorReturn(Function valueSupplier) as well?
  • What about Single.onErrorReturn(Function resumeFunction)?
  • Is it ok to do all of this in the same PR?

@akarnokd
Copy link
Member

akarnokd commented Jul 3, 2019

Do you want to change Maybe.onErrorResumeNext(MaybeSource) as well?

Yes, every class where onErrorResumeNext has a source type and a function.

To what should Observable.onErrorReturn(Function valueSupplier) be renamed?

There is no ambiguity there as the other is called onErrorReturnItem

Do you want to rename Maybe.onErrorReturn(Function valueSupplier) as well?

There is no ambiguity there either.

What about Single.onErrorReturn(Function resumeFunction)?

There is no ambiguity there either.

Is it ok to do all of this in the same PR?

You can rename multiple things within the same PR.

@luis-cortes
Copy link
Contributor

@akarnokd Thanks for the clarification.

Out of curiosity, @vanniktech was there a reason besides ambiguity that made you suggest renaming the onErrorReturns?

luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Observable.onErrorResumeNext to Observable.on…
8639d2a 
…ErrorResumeWith, removing unnecessary cast from null tests, and updating Observable.onErrorResumeWith's JavaDoc to reference correct parameter name, renamed test classes since the distinctions in their names are no longer necessary.
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Maybe.onErrorResumeNext(MaybeSource) to Maybe…
16afaa7 
….onErrorResumeWith(MaybeSource), renamed some of the affected unit tests, and updated JavaDoc.
@luis-cortes
Copy link
Contributor

@akarnokd Is there a reason Single.onErrorResumeNext(final Single<? extends T> resumeSingleInCaseOfError) takes a Single and not a SingleSource like the other methods?

I've made the change and run all the tests for this method and all of them pass.

If there's no reason and it's ok with you I'd be happy to make the change since I'm in the area anyway.

@akarnokd
Copy link
Member

akarnokd commented Jul 3, 2019

Looks like an API mistake. You can change that too (and see if other places need fixing as well).

luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Single.onErrorResumeNext(Single) to Single.on…
5ec1afd 
…ErrorResumeWith(Single), renamed an affected unit test, updated JavaDoc, and removed redundant casts.
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Changing Single.onErrorResumeWith parameter from Singl…
13d5a0f 
…e<? extends T> to SingleSource<? extends T>
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Flowable.onErrorResumeNext(Publisher) to Flow…
3d5fd43 
…able.onErrorResumeWith(Publisher), renaming some affected tests, deleted duplicated unit test that arose from being able to remove redundant casts, updated JavaDocs.
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Observable.onErrorResumeNext to Observable.on…
e749ede 
…ErrorResumeWith, removing unnecessary cast from null tests, and updating Observable.onErrorResumeWith's JavaDoc to reference correct parameter name, renamed test classes since the distinctions in their names are no longer necessary.
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Maybe.onErrorResumeNext(MaybeSource) to Maybe…
9359c2e 
….onErrorResumeWith(MaybeSource), renamed some of the affected unit tests, and updated JavaDoc.
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Single.onErrorResumeNext(Single) to Single.on…
72a20aa 
…ErrorResumeWith(Single), renamed an affected unit test, updated JavaDoc, and removed redundant casts.
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Changing Single.onErrorResumeWith parameter from Singl…
f9fe7ff 
…e<? extends T> to SingleSource<? extends T>
luis-cortes added a commit to luis-cortes/RxJava that referenced this issue Jul 3, 2019
@luis-cortes
ReactiveX#6551 Renaming Flowable.onErrorResumeNext(Publisher) to Flow…
8329dc7 
…able.onErrorResumeWith(Publisher), renaming some affected tests, deleted duplicated unit test that arose from being able to remove redundant casts, updated JavaDocs.
@luis-cortes luis-cortes mentioned this issue Jul 3, 2019
Merged
akarnokd pushed a commit that referenced this issue Jul 4, 2019
@luis-cortes @akarnokd
3.x rename on error resume next methods to disambiguate when calling …
b40a892 
…from kotlin (#6551) (#6556)

* #6551 Renaming Observable.onErrorResumeNext to Observable.onErrorResumeWith, removing unnecessary cast from null tests, and updating Observable.onErrorResumeWith's JavaDoc to reference correct parameter name, renamed test classes since the distinctions in their names are no longer necessary.

* #6551 Renaming Maybe.onErrorResumeNext(MaybeSource) to Maybe.onErrorResumeWith(MaybeSource), renamed some of the affected unit tests, and updated JavaDoc.

* #6551 Renaming Single.onErrorResumeNext(Single) to Single.onErrorResumeWith(Single), renamed an affected unit test, updated JavaDoc, and removed redundant casts.

* #6551 Changing Single.onErrorResumeWith parameter from Single<? extends T> to SingleSource<? extends T>

* #6551 Renaming Flowable.onErrorResumeNext(Publisher) to Flowable.onErrorResumeWith(Publisher), renaming some affected tests, deleted duplicated unit test that arose from being able to remove redundant casts, updated JavaDocs.
@akarnokd akarnokd mentioned this issue Jul 8, 2019
Closed
70 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.x PR welcome
Projects
None yet
Milestone
3.0
3 participants
@akarnokd @vanniktech @luis-cortes