Skip to content

Remove groupBy with selector. #910

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
Merged
benjchristensen merged 1 commit into from
Feb 20, 2014
Merged

Remove groupBy with selector. #910

benjchristensen merged 1 commit into from
Feb 20, 2014

Conversation

@benjchristensen
Copy link
Member

@benjchristensen benjchristensen commented Feb 20, 2014

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

benjchristensen commented Feb 20, 2014

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

benjchristensen commented Feb 20, 2014

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

cloudbees-pull-request-builder commented Feb 20, 2014

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.

3 participants

@benjchristensen @daveray @cloudbees-pull-request-builder