Skip to content

Conversation

@akarnokd
Copy link
Member

Issue #52

  • Added variance to "lambda" parameters
  • Using PublishSubject as it now properly handles subscriptions after its terminal state

Can't do much about the scala tests failing.

@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

@akarnokd While reviewing this @headinthebox and I were comparing with C# and this looks similar so I need to validate the licensing and copyright. Is this copy-paste-modified from the Rx.Net codebase or just inspired by and written from scratch? The Rx.Net code is open source, but I need to validate with legal how to represent the licenses and copyrights if this is from there.

@akarnokd
Copy link
Member Author

Factually, I've written it from scratch based on the join() operator's pattern, but I took a strong inspiration for the join() operator from the Rx.Net sources. I didn't copy-paste the operator's code, but only the test strings from one of the Rx.Net tests of this operator. The operator looks similar, but since GroupedObservable is not an interface in RxJava, I had to work around it with the PublishSubject. Otherwise, I would say the same behavior can be expressed in Java very look-alike. I admit I haven't bothered giving different names to classes and fields. I believe, with the given RxJava style constraints, there was only one way to implement these operators which thereby look almost the same as the Rx.Net versions.

I certainly don't want to fall into an Oracle vs, Google situation, but unfortunately, certain patterns are already established in RxJava. I could have used Locks, ignored the try-catches, used different names, etc.
(When I started my R4J library back in 2011, there was no source code available, no documentation but only a bunch of videos and blog posts: I had no choice but work from them and have a truly "clean room" implementation.) So let's say there weren't any Rx.Net sources available today, but the API documentation would be extensive, I could almost certainly do most operators from scratch, and in the "Java way". But unfortunately, the documentation is inadequate and the aim to produce compatible behavior makes me look at the C# code to figure out the missing logic. I guess half of the RxJava's bugs came from this situation.

I whish to be a valuable part of the RxJava community and see the reactive concept to become mainstream and "standard" in Java. Please advise, given the constraints, how I can avoid such situations in the future.

@benjchristensen
Copy link
Member

Thank you for the details. I'm just being cautious to make sure things are good, and I very much appreciate your involvement.

I'm still clarifying some things with legal to understand what "derivative" means and if we need to include notices, copyrights etc from the C# code.

But unfortunately, the documentation is inadequate and the aim to produce compatible behavior makes me look at the C# code to figure out the missing logic. I guess half of the RxJava's bugs came from this situation.

This is correct, and why now with @headinthebox involved we're fixing nuanced bugs that resulted from desired behavior not being clear in the signature or documentation.

I whish to be a valuable part of the RxJava community and see the reactive concept to become mainstream and "standard" in Java. Please advise, given the constraints, how I can avoid such situations in the future.

Thank you for getting involved and please continue to be so. I'm just making sure we figure this out now rather than down the road.

@akarnokd akarnokd mentioned this pull request Dec 4, 2013
@benjchristensen
Copy link
Member

I've talked with legal and are good to go. Basically, add any notices/copyrights if needed, and if not needed then just the Apache license header like all other files in this project.

Can you rebase this so it can be merged? The master branch has changed enough that this conflicts now.

Thanks.

@akarnokd
Copy link
Member Author

akarnokd commented Dec 4, 2013

Right away.

@akarnokd akarnokd closed this Dec 4, 2013
@akarnokd akarnokd deleted the GroupByUntil4 branch January 13, 2014 10:04
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.

3 participants