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

Ported groupByUntil function to scala-adapter #732

Merged
merged 2 commits into from
Jan 14, 2014

Conversation

chrisgrimm
Copy link

Ported the groupByUntil function to scala-adapter. This is my first time contributing, please let me know if anything is off.

@cloudbees-pull-request-builder

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

@samuelgruetter
Copy link
Contributor

Thanks @chrisgrimm for contributing!

Some comments:

  • If we add wildcards to the Duration Observable in Java (commit), then we can get rid of the cast in Scala (commit).
  • There's an important difference between the signature we have in Java and your signature in Scala: In Java, the durationSelector gets a GroupedObservable, so it can extract the key if it wants to do so, but in Scala, it only gets an Observable, so it cannot know the key.
  • When we add an operator to the Scala Observable, we usually also add an example to RxScalaDemo. These examples needn't test the functionality, because that's already done in Java, but they "test the signature": If you can write a meaningful example, then your signature is good. And these examples are also a useful piece of documentation.

@chrisgrimm
Copy link
Author

@samuelgruetter
Okay, cool. I'll amend my changes to reflect these comments shortly.

@benjchristensen
Copy link
Member

If you get these changes in today I'll release tomorrow (Tuesday US time).

@chrisgrimm
Copy link
Author

@benjchristensen Sorry for the delay I just woke up. I added the example, changed the groupByUntil operator so that the conversion in the scala adapter wasn't necessary, and changed the signature of the closings function to include the key of the group in question.

@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

Thanks @chrisgrimm

benjchristensen added a commit that referenced this pull request Jan 14, 2014
Ported groupByUntil function to scala-adapter
@benjchristensen benjchristensen merged commit d3a7350 into ReactiveX:master Jan 14, 2014
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
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.

4 participants