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

Adding eager concats to Single #5976

Merged
merged 7 commits into from
Apr 29, 2018
Merged

Adding eager concats to Single #5976

merged 7 commits into from
Apr 29, 2018

Conversation

Desislav-Petrov
Copy link
Contributor

@Desislav-Petrov Desislav-Petrov commented Apr 28, 2018

  • Adding concatEager operator for Singles covering:
    concatEager(Publisher<? extends SingleSource<? extends T>> sources)
    concatEager(Iterable<? extends SingleSource<? extends T>> sources)
    concatArrayEager(SingleSource<? extends T>... sources)

  • Issue: Single.concatEager is not implemented #5974

  • Added tests for both methods, I wasn't sure if I should add tests covering just a single element in the vararg list and respectively an iterable with a single element.. any thoughts on this?

@akarnokd akarnokd added this to the 2.2 milestone Apr 28, 2018
public static <T> Flowable<T> concatEager(Iterable<? extends SingleSource<? extends T>> sources) {
return Flowable.fromIterable(sources).concatMapEager(SingleInternalHelper.<T>toFlowable());
}

Copy link
Member

Choose a reason for hiding this comment

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

There is another overload you missed:

public static <T> Flowable<T> concatEager(Publisher<? extends SingleSource<? extends T>> sources)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops - thanks for the review, will update shortly

@akarnokd
Copy link
Member

First of all, there is no reason keep the welcome text in the description.

Added tests for both methods, I wasn't sure if I should add tests covering just a single element in the vararg list and respectively an iterable with a single element.. any thoughts on this?

Since this operator relies on Flowable.concatMapEager, there is no need for too much testing, only ensuring that the Single.concatEager is implemented and Single remains 100% covered.

@codecov
Copy link

codecov bot commented Apr 28, 2018

Codecov Report

Merging #5976 into 2.x will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5976      +/-   ##
============================================
- Coverage     98.27%   98.25%   -0.03%     
- Complexity     6019     6021       +2     
============================================
  Files           656      656              
  Lines         44037    44040       +3     
  Branches       6100     6100              
============================================
- Hits          43278    43271       -7     
- Misses          224      230       +6     
- Partials        535      539       +4
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/reactivex/Maybe.java 100% <ø> (ø) 171 <0> (ø) ⬇️
src/main/java/io/reactivex/Single.java 100% <100%> (ø) 146 <3> (+3) ⬆️
...reactivex/internal/operators/single/SingleAmb.java 96.36% <0%> (-3.64%) 9% <0%> (-1%)
...l/operators/observable/ObservableFlatMapMaybe.java 90.84% <0%> (-3.27%) 2% <0%> (ø)
...tivex/internal/schedulers/TrampolineScheduler.java 96.1% <0%> (-2.6%) 6% <0%> (ø)
.../io/reactivex/internal/schedulers/IoScheduler.java 89.24% <0%> (-2.16%) 9% <0%> (ø)
...nternal/operators/observable/ObservableWindow.java 98% <0%> (-2%) 3% <0%> (ø)
...activex/internal/schedulers/ScheduledRunnable.java 98.07% <0%> (-1.93%) 29% <0%> (-1%)
.../io/reactivex/disposables/CompositeDisposable.java 98.14% <0%> (-1.86%) 39% <0%> (-1%)
...nternal/operators/observable/ObservableCreate.java 97.39% <0%> (-1.74%) 2% <0%> (ø)
... and 19 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 be12fe2...0fc54dd. Read the comment docs.

@@ -344,6 +344,32 @@
return Flowable.fromArray(sources).concatMapEager(SingleInternalHelper.<T>toFlowable());
}

/**
* Concatenates a Publisher sequence of Publishers eagerly into a single stream of values.
Copy link
Member

Choose a reason for hiding this comment

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

SingleSources

Copy link
Member

Choose a reason for hiding this comment

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

Could you also fix the JavaDoc of the Maybe version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, will do

@@ -345,7 +345,7 @@
}

/**
* Concatenates a Publisher sequence of Publishers eagerly into a single stream of values.
* Concatenates a Publisher sequence of Single sources eagerly into a single stream of values.
Copy link
Member

Choose a reason for hiding this comment

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

SingleSources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm.. i can see this "Single sources" instead of SingleSources in a few other places within the Javadoc of the Single class - guess we should fix all?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I think they are remnants from the time when there was no SingleSource interface yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh i see - fixed in all places within the Single class.

@@ -412,7 +412,7 @@
}

/**
* Concatenates a Publisher sequence of Publishers eagerly into a single stream of values.
* Concatenates a Publisher sequence of Maybe sources eagerly into a single stream of values.
Copy link
Member

Choose a reason for hiding this comment

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

Please fix these too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3 of them found and fixed.

@akarnokd
Copy link
Member

Great! Thanks for contributing. Once one of the regular reviewers gives an approval, I'll merge this. (It's midnight over here so that could happen 8-10 hours from now.)

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