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

New operation Finally (issue #43) #196

Merged
merged 7 commits into from
Apr 3, 2013
Merged

Conversation

abliss
Copy link
Contributor

@abliss abliss commented Mar 17, 2013

Hi,

I was looking around for some fun stuff to do for the Netflix Cloud Prize, and I figured I might start by contributing some enhancements to RxJava.

I did my best to match your existing style and practices, but I've doubtless made some mistakes. If you have time to take a quick look and give me some feedback, I'll accept it gratefully.

Cheers,
--abliss

@abliss
Copy link
Contributor Author

abliss commented Mar 17, 2013

This implements issue #43 (but apparently github doesn't autolinkify issue numbers in pull-request titles like it does in comments.)

@cloudbees-pull-request-builder

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

@prabirshrestha
Copy link
Contributor

@cloudbees-pull-request-builder

RxJava-pull-requests #34 FAILURE
Looks like there's a problem with this pull request

@abliss
Copy link
Contributor Author

abliss commented Mar 17, 2013

@prabirshrestha thanks for the comments; I added a static method to Observable.

As for the try/catch, it was not at all clear to me from the MSDN documentation whether Finally would allow the error to propagate or not. I figured it ought to propagate since this matches the semantics of java's "finally" construct.

Or do you mean that Finally should be using try/catch to intercept exceptions in the RxJava framework? This doesn't seem right to me. I would expect we'd rather trust the source Observable to correctly implement all the necessary try/catch blocks and use onError to propagate exceptions, right?

@cloudbees-pull-request-builder

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

@prabirshrestha
Copy link
Contributor

@abliss seems like you forgot to add the non-static method to Observable too.

Even I'm not sure about the try..catch on finally. Will have to check. Hence I had put the ? at the end.

@abliss
Copy link
Contributor Author

abliss commented Mar 17, 2013

Thanks again @prabirshrestha ; I have added it.

@cloudbees-pull-request-builder

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

}

@Test
public void testFinally() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good practice to keep unit tests atomic. Consider splitting this test to two separate ones. First one that checks successfully completed observable and second one would check onError use case.

@prabirshrestha
Copy link
Contributor

@abliss Here is the .net code I checked with.

new[] { 1, 2 }
    .ToObservable()
    .Finally(() =>
        {
            Console.WriteLine("finally block");
            throw new Exception("finally");
        })
    .Subscribe(Console.WriteLine, ex => Console.WriteLine(ex.Message), () => Console.WriteLine("completed"));

Output:

1
2
completed
finally block
(unhanded exception occurs in finally block)

This mean we don't need try..catch on finally but completed should be called before finally.

Splits a compound unit test into to parts.
Uses mockito instead of a bespoke test object.
Removes unused import statements.
Changes the order of the Finally action w.r.t. onComplete/onError.
@abliss
Copy link
Contributor Author

abliss commented Mar 17, 2013

Thanks @prabirshrestha and @mairbek for your comments; I have fixed the issues.

@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

Welcome to the project @abliss and thank you @prabirshrestha and @mairbek for the reviews on this.

@abliss
Copy link
Contributor Author

abliss commented Mar 19, 2013

Thanks @benjchristensen ! If you have any suggestions for how to make this pull request more attractive, or which issues to work on next, I'm all ears. Otherwise I will probably send a pull request for issue #14 next.

@benjchristensen
Copy link
Member

I've had other things taking my time this week so haven't had a chance to review and merge this yet ... I will get to it though. Issue #14 will be good as a next item.

@abliss
Copy link
Contributor Author

abliss commented Mar 22, 2013

No worries at all; I know it's not exactly a high-priority issue.

Actually I realized that #14 requires a Scheduler which seems to be still under discussion. Instead I'm working on an enhancement to Concat (issue #202 ) and will follow that with Switch (issue #13 ).

* @return an Observable that emits the same objects, then calls the action.
* @see <a href="http://msdn.microsoft.com/en-us/library/hh212133(v=vs.103).aspx">MSDN: Observable.Finally Method</a>
*/
public static <T> Observable<T> finally0(Observable source, Action0 action) {
Copy link
Member

Choose a reason for hiding this comment

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

I know we can't have finally as the method name as it is reserved, but finally0 seems awkward. Other than Action0 (because it has different arities) what is the reasoning behind using 0 at the end? I don't think finally will ever have different arities, and if it did it would just have overloads with more arguments.

We likely want to keep the prefix finally since that's what people will be looking for coming from other Rx implementations so perhaps something like finallyDo?

- Changes finally0 to finallyDo.
- Removes unnecessary subscription-wrapping.
- Handle exceptions in onCompleted/onError
@cloudbees-pull-request-builder

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

@abliss
Copy link
Contributor Author

abliss commented Mar 29, 2013

Thanks for the comments; I have fixed them all.

@joshgord joshgord mentioned this pull request Apr 3, 2013
@mattrjacobs mattrjacobs merged commit be560b5 into ReactiveX:master Apr 3, 2013
@mattrjacobs mattrjacobs mentioned this pull request Apr 3, 2013
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.

6 participants