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

Fix javadoc for Observable.reduce() and Observable.reduceWith() #5406

Merged
merged 8 commits into from
Jun 11, 2017

Conversation

devisnik
Copy link
Contributor

@devisnik devisnik commented Jun 11, 2017

The sample code given in the javadoc for Observable.reduce() isn't compiling, since the function returns a Single.
This PR changes it to a compiling example.
It also updates the javadoc for reduceWith() to better match its actual behaviour.

Remark
I'm not sure whether the sample code for reduce() is needed at all. The problem that it addresses might be solved better by using reduceWith(), imho.

Update

  • similar changes applied to Flowable.reduce() and Flowable.reduceWith()

@akarnokd
Copy link
Member

Could you also fix Flowable's javadoc?

@akarnokd akarnokd added this to the 2.2 milestone Jun 11, 2017
@devisnik
Copy link
Contributor Author

@akarnokd Done.
What do you think about referring to reduceWith() instead of giving the example that uses defer()?

@akarnokd
Copy link
Member

The defer example should stay but you can add @see and a 3rd example.

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.

A small original mention of Publisher could be fixed.

*
* // alternatively, by using compose to stay fluent
*
* source.compose(o ->
* Publisher.defer(() -> o.reduce(new ArrayList<>(), (list, item) -> list.add(item)))
* );
* Publisher.defer(() -> o.reduce(new ArrayList<>(), (list, item) -> list.add(item)).toFlowable())
Copy link
Member

Choose a reason for hiding this comment

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

This should be Flowable.defer.

@devisnik
Copy link
Contributor Author

devisnik commented Jun 11, 2017

Updated according to the feedback.

@akarnokd
Copy link
Member

Great!

@codecov
Copy link

codecov bot commented Jun 11, 2017

Codecov Report

❗ No coverage uploaded for pull request base (2.x@c11f715). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             2.x    #5406   +/-   ##
======================================
  Coverage       ?   96.19%           
  Complexity     ?     5789           
======================================
  Files          ?      630           
  Lines          ?    41197           
  Branches       ?     5728           
======================================
  Hits           ?    39629           
  Misses         ?      605           
  Partials       ?      963
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/reactivex/Observable.java 100% <ø> (ø) 506 <0> (?)
src/main/java/io/reactivex/Flowable.java 100% <ø> (ø) 523 <0> (?)

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 c11f715...6757548. Read the comment docs.

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.

2 participants