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.

Sign up for GitHub

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

Jump to bottom

Remove groupBy with selector. #910

Merged
merged 1 commit into from
Feb 20, 2014
Merged

Remove groupBy with selector. #910

merged 1 commit into from
Feb 20, 2014

Conversation

benjchristensen
Copy link
Member

I think we can use groupBy(keySelector).map(elementSelector) instead. Is there any reason to keep this signature?

Related to 02ccc4d#commitcomment-5430646

@benjchristensen
Remove groupBy with selector.
9c50bdc 
Use groupBy.map instead.
benjchristensen referenced this pull request Feb 20, 2014
@benjchristensen
New Bind Signature and GroupBy Operator
02ccc4d 
- Changed `bind` signature to match the variant discussed at #746 (comment)
- Updated code to new signature.
- Re-implemented GroupBy operator with `bind`
@daveray
Copy link
Contributor

daveray commented Feb 20, 2014

Well, groupBy returns Observable<Observable> so you need one more level of map to apply elementSelector. I'm ambivalent.

benjchristensen added a commit that referenced this pull request Feb 20, 2014
@benjchristensen
Merge pull request #910 from benjchristensen/groupBy-selector
c593458 
Remove groupBy with selector.
@benjchristensen benjchristensen merged commit c593458 into ReactiveX:master Feb 20, 2014
@benjchristensen
Copy link
Member Author

Merging for now to remove the broken functionality ... can be re-added before 0.17.0 release if we decide to.

@benjchristensen benjchristensen deleted the groupBy-selector branch February 20, 2014 17:27
@benjchristensen
Copy link
Member Author

Well, groupBy returns Observable so you need one more level of map to apply elementSelector. I'm ambivalent.

Yes sorry, it's within the map or flatMap, but we already have to do that anyways when using groupBy as that's what it does, emit Observable<GroupedObservable<T>>>. I don't see the need for a special operator that emits Observable<GroupedObservable<R>>>.

Thanks @daveray for the feedback. Anyone else have a good reason for the existence of this overload? The preference is allow composition of existing operators.

@cloudbees-pull-request-builder
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@benjchristensen @daveray @cloudbees-pull-request-builder