Window Operator #349

benjchristensen · 2013-09-05T06:21:41Z

Manual merge of #337

Since this pull request was submitted there were significant changes to the create operator and generics, so I manually merged into master. I have not actually reviewed for behavior, but unit tests are passing and code structure looked good.

This is for issue #102

This is from commit http://github.com/RallySoftware/RxJava/commit/71335e4ed1531b1542c7b304a6d3337145e0b2df

…Java - updated to use new OnSubscribeFunc - updated to use super/extends generics

benjchristensen · 2013-09-05T06:24:08Z

@bcotton Great pull request, I like how you also worked on buffer.

Now that I've merged this with master I'll add some inline comments on the code with questions. If another pull request is needed so you can take back control of the branch, go right ahead and just cherry pick these 2 commits on to a new branch of your own.

benjchristensen · 2013-09-05T06:26:44Z

rxjava-core/src/main/java/rx/Observable.java

+     *         An {@link Observable} which produces connected non-overlapping windows containing at most
+     *         "count" produced values.
+     */
+    public Observable<Observable<T>> window(Observable<T> source, int count) {


Now that we're doing covariant super/extend generics, do we need to look at any of these Observable<T> and change them to Observable<? super T>?

This takes an Observable<T>, instead it would have take an Observable<? extends T> to ensure covariance of Observable. Normally, the return type shouldn't be affected.

cloudbees-pull-request-builder · 2013-09-05T06:29:07Z

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

benjchristensen · 2013-09-05T06:33:28Z

rxjava-core/src/main/java/rx/util/Closing.java

@@ -15,6 +15,6 @@
 */
 package rx.util;

-public interface BufferClosing {
+public interface Closing {


I am still not a fan of these random classes/interfaces sitting in rx.util, but I can't think of anything better to do with them.

I don't really want them in rx and they don't seem to warrant their own package, but they seem different than util.

We could put them in Observable as inner classes but that may be a bad precedent if there are more of these cases down the road.

Does anyone else have a better idea of what to do with them?

I don't mind much about them staying in rx.util for the timing being. I definitely wouldn't want them directly in rx, and Observable is already a monster. If somebody comes up with a great name for a package to put them into, I'm all for moving them, but otherwise, I'd leave them as is.

benjchristensen · 2013-09-05T06:42:03Z

This looks like a great addition of code, and not a trival one, so thank you @bcotton!

I do not have the time to review it for thread-safety but nothing jumps out as obviously wrong.

The unit tests appear to cover the use cases well and based on them the behavior looks correct.

I have not done a comparison with Rx.Net to determine if we have covered all cases. If anyone else could do that I'd appreciate it.

Once we've got resolutions to the (1) generics and (2) Func0 questions I posted above, this appears good to merge unless anyone else can find issues.

benjchristensen · 2013-09-05T16:25:45Z

If possible I'd like to get the above items resolved today so I can merge this and release 0.12 by end of today (US time).

jmhofer · 2013-09-05T16:47:44Z

I know that I'm being kind of a pain in the butt here, but there's also the question whether the various Chunk and Window classes are co-/contra- or invariant in their type parameters...

I'll go examine the code more closely now...

jmhofer · 2013-09-05T16:57:49Z

I made the wi(n)dow maker typesafe. - There are also some more raw types in ChunkedOperation, though.

…/extends fluff; also simplified api by removing a few useless super definitions (there's no super of Opening and Closing)

jmhofer · 2013-09-05T17:52:27Z

Now I think I'm done with the important things. The chunks look invariant to me at first sight. I tried to get everything typesafe that wasn't already, and again added a lot of wildcards... - it's all here

@benjchristensen Should I put up a pull request against your window-merge branch?

benjchristensen · 2013-09-05T18:05:16Z

I just pulled them off your branch to this pull request.

Looks like both of my questions/concerns are now addressed.

Any last thoughts on placement of util classes?

Otherwise this looks ready to merge (if the build passes).

cloudbees-pull-request-builder · 2013-09-05T18:11:18Z

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

benjchristensen · 2013-09-05T20:58:15Z

Thanks for the reviews @jmhofer.

@bcotton It looks like this pull request only has you as the author but not commiter due to the crazy merge I had to do. If you want to take these commits and reapply them so you get the credit, please do so.

I'll wait a few hours if you can do it, otherwise I'll proceed with this pull request.

bcotton · 2013-09-05T21:52:14Z

Go ahead. No need for glory :-) Happy to help

benjchristensen · 2013-09-05T22:13:58Z

Thanks @bcotton ... I'll merge now and release 0.12 fairly soon.

Window Operator

samuelgruetter · 2013-09-06T15:12:14Z

I'm confused. These methods all are instance methods, but they do not depend in any way on this, so they could be made static. However, the better way would probably be to remove the argument called source and replace all occurences of source by this.
Am I missing something?

benjchristensen · 2013-09-06T16:17:16Z

No you're not missing anything ... we all did who reviewed this! So funny ... an earlier review had the methods change from static to instance, but apparently only the static keyword got removed.

I'll go submit a fix and release 0.12.1 ... too funny (and embarrassing) we all missed this.

Thanks for reporting it.

jmhofer · 2013-09-06T16:41:55Z

Oops, embarrassing. :-) Completely missed this, too.

benjchristensen · 2013-09-06T17:02:44Z

Just release 0.12.1 ... making its way through Sonatype now.

Window Operator

- reported at ReactiveX#349 (comment)

Bob Cotton and others added 2 commits September 4, 2013 23:07

Merge window operator commit to master

5789894

This is from commit http://github.com/RallySoftware/RxJava/commit/71335e4ed1531b1542c7b304a6d3337145e0b2df

Merge branch 'OperationWindow' of https://github.com/RallySoftware/Rx…

09cac83

…Java - updated to use new OnSubscribeFunc - updated to use super/extends generics

benjchristensen mentioned this pull request Sep 5, 2013

Added the window() operation. #337

Closed

benjchristensen reviewed Sep 5, 2013
View reviewed changes

benjchristensen mentioned this pull request Sep 5, 2013

Idiomatic Scala Support #336

Closed

made WINDOW_MAKER typed

959b0d8

jmhofer added 2 commits September 5, 2013 19:25

removed raw Func0s from the chunks

a7b40b3

made the public window methods more generic via the basic (lol) super…

92fe7f9

…/extends fluff; also simplified api by removing a few useless super definitions (there's no super of Opening and Closing)

benjchristensen added a commit that referenced this pull request Sep 5, 2013

Merge pull request #349 from benjchristensen/window-operator

2221198

Window Operator

benjchristensen merged commit 2221198 into ReactiveX:master Sep 5, 2013

benjchristensen mentioned this pull request Sep 6, 2013

Fix Observable.window static/instance bug #351

Merged

rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014

Merge pull request ReactiveX#349 from benjchristensen/window-operator

bb8acc5

Window Operator

rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014

Fix Observable.window static/instance bug

09fe58d

- reported at ReactiveX#349 (comment)

billyy pushed a commit to billyy/RxJava that referenced this pull request Jan 13, 2014

Fix Observable.window static/instance bug

2e18a2b

- reported at ReactiveX#349 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Window Operator #349

Window Operator #349

benjchristensen commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

benjchristensen Sep 5, 2013

jmhofer Sep 5, 2013

cloudbees-pull-request-builder commented Sep 5, 2013

benjchristensen Sep 5, 2013

jmhofer Sep 5, 2013

benjchristensen commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

jmhofer commented Sep 5, 2013

jmhofer commented Sep 5, 2013

jmhofer commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

cloudbees-pull-request-builder commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

bcotton commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

samuelgruetter commented Sep 6, 2013

benjchristensen commented Sep 6, 2013

jmhofer commented Sep 6, 2013

benjchristensen commented Sep 6, 2013

Window Operator #349

Window Operator #349

Conversation

benjchristensen commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

benjchristensen Sep 5, 2013

Choose a reason for hiding this comment

jmhofer Sep 5, 2013

Choose a reason for hiding this comment

cloudbees-pull-request-builder commented Sep 5, 2013

benjchristensen Sep 5, 2013

Choose a reason for hiding this comment

jmhofer Sep 5, 2013

Choose a reason for hiding this comment

benjchristensen commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

jmhofer commented Sep 5, 2013

jmhofer commented Sep 5, 2013

jmhofer commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

cloudbees-pull-request-builder commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

bcotton commented Sep 5, 2013

benjchristensen commented Sep 5, 2013

samuelgruetter commented Sep 6, 2013

benjchristensen commented Sep 6, 2013

jmhofer commented Sep 6, 2013

benjchristensen commented Sep 6, 2013