Skip to content

1.x: add missing backpressure descriptions and update old ones #4172

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

Merged
merged 2 commits into from
Jul 7, 2016

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Jul 7, 2016

This PR adds or updates places where the Backpressure: description was missing or outdated.

Unfortunately, GitHub has a hard limit on diff size per file so reviewers have to use their own local diff.

@DavidMGross
Copy link
Collaborator

"...it <t>may</i> lead to IllegalStateException being throw..."
=> "...it <em>may</em> lead to IllegalStateException being thrown..."

...but better yet would be to avoid the passive voice and indicate what entity throws the exception, e.g. "If any of the source Observables violate this, XXX may throw an IllegalStateException when the source Observable completes."

@DavidMGross
Copy link
Collaborator

"...no backpressue applied to them..." => "...no backpressure applied to them..."

@DavidMGross
Copy link
Collaborator

"The other Observables supplied by the function is consumed..." => "The other Observables supplied by the function are consumed..."

@DavidMGross
Copy link
Collaborator

I notice that some of the <dd>s aren't being closed with </dd> also.

@DavidMGross
Copy link
Collaborator

typo: "...apply any of the onBackpressureXXX opertors <b>before</b> applying observeOn itself..." => "...apply any of the onBackpressureXXX operators <strong>before</strong> applying observeOn itself..."

@DavidMGross
Copy link
Collaborator

For operators that both accept and return an Observable (most of them), I find the phrase "this Observable" in the documentation to be ambiguous: does it mean the Observable about to be operated on, or the Observable that results from the operation? I'd recommend replacing it with "the source Observable" or "the resulting Observable" or something like that, depending on which one you're talking about.

@DavidMGross
Copy link
Collaborator

DavidMGross commented Jul 7, 2016

typo: "...the output's backpressure behavior is derermined by..." => "...the output's backpressure behavior is determined by..."

@DavidMGross
Copy link
Collaborator

+     *  <dt><b>Backpressure:</b></dt>
+     *  <dd>The operator honors backpressure from downstream and consumes this {@code Observable} in an
+     *  unbounded manner (i.e., no backpressure is applied to it).</dd>
+     *  behavior.</dd>

That last line seems to be extraneous.

@DavidMGross
Copy link
Collaborator

Some of those typos appear in multiple places in the file.

It's great to see these sections getting filled out and made more precise!

@akarnokd
Copy link
Member Author

akarnokd commented Jul 7, 2016

Thanks for the review. I've updated the text.

@codecov-io
Copy link

codecov-io commented Jul 7, 2016

Current coverage is 81.46%

Merging #4172 into 1.x will increase coverage by 0.04%

@@                1.x      #4172   diff @@
==========================================
  Files           257        257          
  Lines         16823      16823          
  Methods           0          0          
  Messages          0          0          
  Branches       2550       2550          
==========================================
+ Hits          13697      13705     +8   
+ Misses         2226       2225     -1   
+ Partials        900        893     -7   

Powered by Codecov. Last updated by cec8915...4428b29

@akarnokd
Copy link
Member Author

akarnokd commented Jul 7, 2016

I'm merging this so work touching Observable.java can commence. Anybody let me know if you find something else (or better yet, post a PR).

@akarnokd akarnokd merged commit 20ef857 into ReactiveX:1.x Jul 7, 2016
@akarnokd akarnokd deleted the ObservableJavadoc707 branch July 7, 2016 20:09
@artem-zinnatullin
Copy link
Contributor

Thank you for this @akarnokd!

@davidmoten
Copy link
Collaborator

Beaut!

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.

5 participants