-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added a partition
method to all containers
#1916
Conversation
partition
method to all containerspartition
method to all containers
Il does seems nice. I will check that. |
Thanks! I think I should draw your attention to the change in |
@@ -389,6 +389,18 @@ export function groupByFactory(collection, grouper, context) { | |||
return groups.map(arr => reify(collection, coerce(arr))).asImmutable(); | |||
} | |||
|
|||
export function partitionFactory(collection, predicate, context) { | |||
const isKeyedIter = isKeyed(collection); | |||
const groups = [[], []]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of using a immutable Set for the main container ?
It seems coherent with the groupBy function that returns an immutable instance ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Set has the wrong semantics because the result is ordered. As for using something else like a List, there are a couple of reasons why I didn't do it that way. One is that I expect the array to be immediately destructured in typical use, so creating an instance of an immutable type would add extra overhead that rarely has any benefit. The other is that it would break the TypeScript type of the result. In TypeScript the result is guaranteed to have exactly two values, and they even have different types when the predicate is a type-testing function.
I think the only other type that really makes sense is a native object type with fields named something like true
and false
; it would make the meaning the fields more clear, and it has no more overhead than an array. (I checked, and "true" and "false" work fine as field names.) Now that I think of it, I think returning an object is probably the the best, most idiomatic solution all around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I like the array better than the object, because object with true
and false
keys, even if they work, might be misinterpreted.
Moreover, you can't do stuff like that:
const { false, true } = partitionned;
In that case, your implementation seems a good approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. I'm happy to leave it as-is.
@johnw42 it has been release in |
This PR adds the
partition
method common in other libraries and languages to all immutable containers. It contains the implementation as well as updated documentation, TypeScript types, and unit tests. I have not added Flow support because I'm not familiar enough with it. See changes toREADMD.md
for more details.