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

Update according to review in issue #1159 #1165

Merged
merged 3 commits into from
May 8, 2014

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented May 7, 2014

@cloudbees-pull-request-builder

RxJava-pull-requests #1080 SUCCESS
This pull request looks good

@@ -2580,7 +2580,7 @@ trait Observable[+T]
* <p>
* <img width="640" src="https://raw.github.com/wiki/Netflix/RxJava/images/rx-operators/doOnTerminate.png">
* <p>
* This differs from `finallyDo` in that this happens BEFORE onCompleted/onError are emitted.
* This differs from `finallyDo` in that this happens **before** `onCompleted/nError` are emitted.
Copy link
Contributor

Choose a reason for hiding this comment

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

nError => onError ;-)

@zsxwing zsxwing mentioned this pull request May 7, 2014
@zsxwing
Copy link
Member Author

zsxwing commented May 7, 2014

@benjchristensen could you wait for this PR ready and merge it before you release 0.18.3?

@cloudbees-pull-request-builder

RxJava-pull-requests #1082 SUCCESS
This pull request looks good

@@ -167,7 +167,7 @@ class CompletenessTest extends JUnitSuite {
"zip(Iterable[_ <: Observable[_]], FuncN[_ <: R])" -> "[use `zip` in companion object and `map`]"
) ++ List.iterate("T", 9)(s => s + ", T").map(
// all 9 overloads of startWith:
"startWith(" + _ + ")" -> "[unnecessary because we can just use `::` instead]"
"startWith(" + _ + ")" -> "[unnecessary because we can just use `++` instead]"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could differentiate here: instead of startWith(T), use +:, and instead of startWith(more than one T), use Observable.items(...) ++ o

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Already updated it.

@cloudbees-pull-request-builder

RxJava-pull-requests #1083 SUCCESS
This pull request looks good

@@ -167,7 +167,7 @@ class CompletenessTest extends JUnitSuite {
"zip(Iterable[_ <: Observable[_]], FuncN[_ <: R])" -> "[use `zip` in companion object and `map`]"
) ++ List.iterate("T", 9)(s => s + ", T").map(
// all 9 overloads of startWith:
"startWith(" + _ + ")" -> "[unnecessary because we can just use `::` instead]"
"startWith(" + _ + ")" -> "[unnecessary. Instead of `startWith(T)`, use `+:`, and instead of `startWith(more than one T)`, use `Observable.items(...) ++ o`]"
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of this table is to have one row for each item, so you should split this into two. Also, when editing this code, from time to time you should run printMarkdownCorrespondenceTable, and convert the markdown to html and check if the generated html looks nice.
And you're also welcome to make PRs against https://github.com/RxScala/RxScala.github.io so that your updates are actually read by visitors of the http://rxscala.github.io/comparison.html page ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it. I'll send a PR to RxScala.github.io once this one is merged.

@cloudbees-pull-request-builder

RxJava-pull-requests #1084 SUCCESS
This pull request looks good

benjchristensen added a commit that referenced this pull request May 8, 2014
Update according to review in issue #1159
@benjchristensen benjchristensen merged commit f00cba9 into ReactiveX:master May 8, 2014
@zsxwing zsxwing deleted the review-issue1159 branch May 8, 2014 12:32
@benjchristensen benjchristensen mentioned this pull request Jun 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants