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

fix(partition): update signature to match docs and filter operator #2819

Merged
merged 2 commits into from
Sep 19, 2017

Conversation

manbearwiz
Copy link
Contributor

Description:

This PR adds the index parameter to the predicate signature for the partition operator. This change matches the docs as well as the existing filter operator.

@kwonoj
Copy link
Member

kwonoj commented Aug 30, 2017

Interesting, it seems like we had signature to support index but removed it for some reason, not sure why.

Still, it already makes current public api surface as-is, means if we make index as non-optional param, it'll be breaking changes for next major instead of minor. (next branch.) For me it seems more feasible to make index to optional.

@kwonoj
Copy link
Member

kwonoj commented Aug 30, 2017

/cc @david-driscoll as well.

@manbearwiz
Copy link
Contributor Author

manbearwiz commented Aug 30, 2017

Yah I saw the commit that removed it but assumed it was removed on accident.

As for it being a surface api change, it may not matter in this case. There is no change in the way the predicate is called in the filter operator so if the given predicate does not have the index in its signature it will just be ignored. All of the existing tests in the spec show this.

@rxjs-bot
Copy link

Messages
📖

CJS: 3735.2KB, global: 707.3KB (gzipped: 133.8KB), min: 140.2KB (gzipped: 30.0KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage remained the same at 97.395% when pulling 2460114 on manbearwiz:patch-1 into d926435 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Sep 4, 2017

I'm not sure this is a breaking change, @kwonoj.(x: T) => R is compatable with (x: T, i: number) => R as an argument.

@kwonoj
Copy link
Member

kwonoj commented Sep 4, 2017

yeah, I think I misread signatures.

@benlesh benlesh merged commit 755df9b into ReactiveX:master Sep 19, 2017
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants