Adding eager concats to Single by Desislav-Petrov · Pull Request #5976 · ReactiveX/RxJava · GitHub
Skip to content

Adding eager concats to Single#5976

Merged
akarnokd merged 7 commits into
ReactiveX:2.xfrom
Desislav-Petrov:2.x
Apr 29, 2018
Merged

Adding eager concats to Single#5976
akarnokd merged 7 commits into
ReactiveX:2.xfrom
Desislav-Petrov:2.x

Conversation

@Desislav-Petrov

@Desislav-Petrov Desislav-Petrov commented Apr 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

  • 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
akarnokd
akarnokd reviewed Apr 28, 2018
View reviewed changes
Comment thread src/main/java/io/reactivex/Single.java

@akarnokd akarnokd Apr 28, 2018

Copy link
Copy Markdown
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)

@Desislav-Petrov Desislav-Petrov Apr 28, 2018

Copy link
Copy Markdown
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

akarnokd commented Apr 28, 2018

Copy link
Copy Markdown
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

codecov Bot commented Apr 28, 2018 •
edited
Loading

Copy link
Copy Markdown

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.

akarnokd
akarnokd reviewed Apr 28, 2018
View reviewed changes
Comment thread src/main/java/io/reactivex/Single.java Outdated
}

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

@akarnokd akarnokd Apr 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SingleSources

@akarnokd akarnokd Apr 28, 2018

Copy link
Copy Markdown
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?

@Desislav-Petrov Desislav-Petrov Apr 28, 2018

Copy link
Copy Markdown
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

akarnokd
akarnokd reviewed Apr 28, 2018
View reviewed changes
Comment thread src/main/java/io/reactivex/Single.java Outdated

/**
* 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.

@akarnokd akarnokd Apr 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SingleSources

@Desislav-Petrov Desislav-Petrov Apr 28, 2018

Copy link
Copy Markdown
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?

@akarnokd akarnokd Apr 28, 2018

Copy link
Copy Markdown
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.

@Desislav-Petrov Desislav-Petrov Apr 28, 2018

Copy link
Copy Markdown
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.

akarnokd
akarnokd reviewed Apr 28, 2018
View reviewed changes
Comment thread src/main/java/io/reactivex/Maybe.java Outdated

/**
* 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.

@akarnokd akarnokd Apr 28, 2018

Copy link
Copy Markdown
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.

@Desislav-Petrov Desislav-Petrov Apr 28, 2018

Copy link
Copy Markdown
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

akarnokd commented Apr 28, 2018

Copy link
Copy Markdown
Member

@akarnokd akarnokd merged commit 99366ee into ReactiveX:2.x Apr 29, 2018
@akarnokd akarnokd mentioned this pull request Apr 29, 2018
Closed
@akarnokd akarnokd mentioned this pull request May 17, 2018
Closed
@jared-bot jared-bot mentioned this pull request May 23, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akarnokd akarnokd akarnokd approved these changes
+1 more reviewer
@vanniktech vanniktech vanniktech approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.x Enhancement

Projects

None yet

Milestone

2.2

Development

Successfully merging this pull request may close these issues.

3 participants

@Desislav-Petrov @akarnokd @vanniktech