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

2.x: Add note about NoSuchElementException to Single.zip(). #5876

Conversation

artem-zinnatullin
Copy link
Contributor

No description provided.

Copy link
Member

@akarnokd akarnokd left a comment

Choose a reason for hiding this comment

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

There is no such requirement that these should be not empty, only consequence of being empty.

@@ -1294,7 +1296,7 @@
* </dl>
* @param <T> the common value type
* @param <R> the result value type
* @param sources the Iterable sequence of SingleSource instances
* @param sources the Iterable sequence of SingleSource instances, must not be empty
Copy link
Member

Choose a reason for hiding this comment

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

"An empty sequence will result in an onError signal of NoSuchElementException."

@@ -1736,7 +1740,7 @@
* </dl>
* @param <T> the common value type
* @param <R> the result value type
* @param sources the array of SingleSource instances
* @param sources the array of SingleSource instances, must not be empty
Copy link
Member

Choose a reason for hiding this comment

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

"An empty array will result in an onError signal of NoSuchElementException."

@akarnokd akarnokd added this to the 2.2 milestone Mar 2, 2018
@@ -1279,6 +1279,8 @@
* value and calls a zipper function with an array of these values to return a result
* to be emitted to downstream.
* <p>
* If the {@code Iterable} of {@link SingleSource}s is empty a {@link NoSuchElementException} error is signalled after subscription.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add explanation if you think it adds value, something like:

/**
 * Since {@link Single} can't complete without value, there is nothing zip operator
 * can do about zipping 0 {@code Single}s.
 */

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't sound good to me. Simply state the fact that empty sequence results in a NoSuchElementException.

@akarnokd
Copy link
Member

akarnokd commented Mar 2, 2018

This may be a desirable documentation enhancement the other zip operators all around. Would you like to do them as well in this or another PR?

@artem-zinnatullin
Copy link
Contributor Author

Yep, I'll check other rx types to update that, for some types like Observable it's fine since they can just signal completion on empty input.

Will post comment with update in any case :)

@codecov
Copy link

codecov bot commented Mar 2, 2018

Codecov Report

Merging #5876 into 2.x will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5876      +/-   ##
============================================
+ Coverage     96.59%   96.64%   +0.04%     
- Complexity     5893     5894       +1     
============================================
  Files           652      652              
  Lines         43403    43403              
  Branches       6033     6033              
============================================
+ Hits          41926    41946      +20     
+ Misses          567      555      -12     
+ Partials        910      902       -8
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/reactivex/Single.java 100% <ø> (ø) 142 <0> (ø) ⬇️
...l/operators/observable/ObservableFlatMapMaybe.java 84.96% <0%> (-7.19%) 2% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 61.8% <0%> (-2.78%) 35% <0%> (-2%)
...ernal/operators/flowable/FlowableFromIterable.java 95.18% <0%> (-1.61%) 5% <0%> (ø)
.../operators/maybe/MaybeFlatMapIterableFlowable.java 97.54% <0%> (-0.82%) 2% <0%> (ø)
...rnal/operators/observable/ObservableSwitchMap.java 90% <0%> (-0.63%) 3% <0%> (ø)
...internal/operators/flowable/FlowableObserveOn.java 96.53% <0%> (-0.58%) 3% <0%> (ø)
...x/internal/operators/flowable/FlowableGroupBy.java 95.4% <0%> (-0.58%) 3% <0%> (ø)
...rnal/operators/flowable/FlowableFlatMapSingle.java 92.93% <0%> (-0.55%) 2% <0%> (ø)
...ex/internal/operators/flowable/FlowableCreate.java 97.74% <0%> (+0.32%) 6% <0%> (ø) ⬇️
... and 15 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 d3ed269...d503378. Read the comment docs.

@artem-zinnatullin
Copy link
Contributor Author

@akarnokd in internal discussion at work with @kxfang we've come to conclusion with that Single.zip(Iterable) and Single.zipArray() should return Maybe to turn this case into normal completion event

If you think it's reasonable let's add it to the list of breaking 3.x changes #5622?

@akarnokd
Copy link
Member

akarnokd commented Mar 2, 2018

An empty input sequence to zip sounds more like a mistake to be warned about instead of a feature.

@artem-zinnatullin
Copy link
Contributor Author

So I briefly checked zip*(Iterable/Array) overloads of :

  • Observable
  • Flowable
  • Maybe
  • Completable

And they emit completion if input is an empty collection of sources.

I agree that if you care about emitted value, "silent" completion is not great.
And if you don't care about value, you probably should convert result of zip to Completable to make it explicit.
However to convert result to Completable you need to be able to do it naturally (.toCompletable()), which is not consistent with current Single.zip() behavior because it emits error.

For consistency with other rx types I'd still suggest to return Maybe as a result of Single.zip(Iterable/Array)

An alternative would be to emit error from other rx types in this scenario, but such change doesn't sound pragmatic

Note:

Completable.zip() doesn't exist because there are no values to zip, but merge() basically does zip() in case of Completable.

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