Proposal: std/collections - first part #993

kt3k · 2021-06-30T14:52:09Z

This proposal suggests adding 17 APIs (11 Array utils, 6 Record utils) as std/collections module. (These are extracted from the original proposal #978 by @LionC. We keep discussion on other APIs on the original PR.)

Array utils (11)

distinct
distinctBy
intersect
union
findLast
zip
unzip
groupBy
partition
chunked
permutations

Record utils (6)

filterEntries
filterKeys
filterValues
mapEntries
mapKeys
mapValues

In the original issue, nobody was against adding these 17. So this can be a minimal set of APIs which we could land as a first pass of std/collections.

I'd like to land these items first if there's no strong objections to some specific items or the addition of std/collections entirely.

Adds test cases for the `union function. Also applied formatting on the directory, also refactored existing test cases to use the same convention on naming tests etc. Adds the copyright headers to all files in the module.

ry

This looks like a good set of APIs to start with.

wperron · 2021-07-07T18:59:23Z

I started implementing union and intersect one think I'm noticing is that we could change the input parameters from a: Array<T>, b: Array<T> to ...a: Array<Array<T>> so that it can be called on an arbitrary number of arrays at once.

collections/union.ts

LionC · 2021-07-09T14:08:39Z

I like the varargs for union and intersect.

I am back from vacation now and will continue implementing :-)

Zzzen · 2021-07-09T15:25:10Z

collections/distinct.ts

+ * const numbers = [ 3, 2, 5, 2, 5 ]
+ * const distinctNumbers = distinct(numbers)
+ *
+ * console.assert(distinctNumbers === [ 3, 2, 5 ])


nitpick: console.assert will always fail because their identities are different. assert.deepStrictEqual(distinctNumbers, [3,2,5]) in nodejs or expect(distinctNumbers).toEqual([3,2,5]) in jest might be a better choice.

You are absolutely right - we should make sure that those examples actually work. I just totally missed that. I will put it on my to do list to do a sweep of that in the end.

LionC · 2021-07-14T21:36:21Z

I am finished with implementations and tests for now - obviously pending review. Will move on to refining types and docs now.

sebastienfilion · 2021-07-14T22:20:12Z

Amazing work!

I was wondering if there’d be a lot of opposition to sticking to intransitive verbs for the functions. I am confused by chunked and permutations — why isn’t it chunk and permute?
Those two specifically led me off their true intent.

Then there’s union which definitely is more common… although it could be named concat, or concatUnique since it seems to remove duplicates… but I don’t have a strong opinion about that last one.

LionC · 2021-07-14T22:22:19Z

I was wondering if there’d be a lot of opposition to sticking to intransitive verbs for the functions. I am confused by chunked and permutations — why isn’t it chunk and permute?
Those two specifically led me off their true intent.

Then there’s union which definitely is more common… although it could be named concat, or concatUnique since it seems to remove duplicates… but I don’t have a strong opinion about that last one.

To be honest, names are mostly stolen from kotlin/collections and lodash, which seem to have a very similiar idea about this. As they are both used a lot and I personally never had an issue with the names, I did not modify them too much.

getspooky · 2021-07-14T22:43:01Z

Amazing work 👍,
I think it would be helpful to add this collections of 17 APIs (11 Array utils, 6 Record utils) into Third Party Modules
GitHub instead of Standard Library because it's not related to deno but to typescript.

LionC · 2021-07-14T22:51:40Z

Amazing work 👍,

I think it would be helpful to add this collections of 17 APIs (11 Array utils, 6 Record utils) into [Third Party Modules

GitHub](https://deno.land/x) instead of Standard Library because it's not related to deno but to typescript.

As I understand it, the std lib tries to battle the dependency madness of needing third party code for everything, which is why we decided to do it this way. There has been a lot of discussion around this module on Discord and in the linked Discussions (following the links in the initial post of this PR).

wperron

I have some comments, most of them are minor nitpick, and obviously there needs to be some more docs. Also make sure that the block comments are formatted to stick within 80 characters and fix the linting errors.

Other than that, great work, looking forward to landing this!

wperron · 2021-07-15T17:43:31Z

collections/distinct_by_test.ts

+});
+
+Deno.test({
+  name: "[collections/distinctBy] gets head on noop selector",


This one feels a bit weird to me; If there are no conditions on which to identify distinct item, how do we determine that they are all equivalent? Shouldn't this return a copy of the initial array instead?

As () => {} is equivalent to it => undefined here, it would feel like unexpected magic behavior to me if it would explicitly ignore equality (distinctity? is that a word?) for undefined. What about null? ""? What if undefined is a legit value in my model?

I wrote those tests before implementing the function, so to mee it seemed like the intuitive behavior to just distinct all values by their produced value - no matter what it is.

collections/distinct_by_test.ts

wperron · 2021-07-15T17:51:38Z

collections/distinct.ts

+export function distinct<T>(array: Array<T>): Array<T> {
+  const set = new Set(array);
+
+  return Array.from(set);


Could be inlined to return Array.from(new Set(array));

@wperron #1099

wperron · 2021-07-15T17:56:00Z

collections/filter_entries_test.ts

+          "b": "foo",
+          "C": 5,
+          "d": -2,
+          "": false,


Having a case here where the same letter appears in both upper and lower would drive the point home a little better, otherwise nice test

I agree - I cannot add it on vacation, feel free to do it or I will do it once I return :-)

wperron · 2021-07-15T17:59:15Z

collections/filter_entries.ts

+  const ret: Record<string, T> = {};
+  const entries = Object.entries(record);
+
+  for (const [key, value] of entries) {


Could inline to for (const [key, value] of Object.entries(record)) {

collections/map_entries.ts

collections/map_keys.ts

collections/map_values.ts

collections/unzip.ts

collections/_utils.ts

ry

LGTM - thank you @LionC !

Good enough for a first pass, we can clean up in the future as necessary

x

Co-authored-by: William Perron <hey@wperron.io>

chore: add std/collections placeholder

9533251

ry requested a review from wperron July 1, 2021 23:14

LionC force-pushed the std-collections-part1 branch 2 times, most recently from 962800c to b0f1047 Compare July 2, 2021 00:37

LionC added 2 commits July 2, 2021 02:51

add dummy implementation and tests for distinct functions

09e9a36

add todo comments to stub implementations

41c9a47

LionC force-pushed the std-collections-part1 branch from 59f704e to 41c9a47 Compare July 2, 2021 00:51

wperron and others added 3 commits July 2, 2021 16:42

add union test cases and misc changes

1d65acb

Adds test cases for the `union function. Also applied formatting on the directory, also refactored existing test cases to use the same convention on naming tests etc. Adds the copyright headers to all files in the module.

rename union to intersection

86dba04

Merge branch 'main' into std-collections-part1

bb882a1

ry reviewed Jul 5, 2021

View reviewed changes

wperron added 2 commits July 7, 2021 14:47

Merge remote-tracking branch 'upstream/main' into std-collections-part1

84396a8

add test cases and misc changes

7cebd65

implement union

903b7cb

Zzzen reviewed Jul 9, 2021

View reviewed changes

collections/union.ts Outdated Show resolved Hide resolved

make union side effect free

a100d64

Zzzen reviewed Jul 9, 2021

View reviewed changes

kt3k and others added 11 commits July 12, 2021 20:37

separate files

0c96c81

add internal collection _utils with filterInPlace

86d2d85

implement intersect and fix tests

a73d749

add intersect no mutation test

41aab80

add tests for collections/chunked

5a86b89

implement collections/chunked

04061a5

change collections/distinct/nans test to reflect Set equality

90a25e7

implement collections/distinct

bdd3beb

change collections/distinctBy/nans test to reflect Set equality

3eb472b

implement collections/distinctBy

fd103f2

add tests for collections/findLast

fd624a9

LionC added 11 commits July 14, 2021 14:59

add tests for collections/mapValues

2265186

implement collections/mapvalues

65c6226

add tests for collections/permutations

a7b6ae3

implement collections/permutations

820a4f7

unify for of usage and formatting for collections record functions

c36c13a

refactor union to accept varargs

31ca013

make collections/distinctBy use for of

d061653

remove unnecessary empty check for collections/findLast

d222baa

make collections/groupBy use for of

18c54df

fix collections/intersection varargs empty behaviour and adapt tests

860a5df

make collections/partition use for of

c394ac6

use relative import for testing

9cea788

wperron previously requested changes Jul 15, 2021

View reviewed changes

ry added 2 commits July 15, 2021 17:22

lint

29dadef

remove duplicate test

ecbac6d

ry approved these changes Jul 15, 2021

View reviewed changes

ry merged commit a23eb70 into denoland:main Jul 15, 2021

ry mentioned this pull request Jul 15, 2021

std / collections #978

Closed

5 tasks

kt3k deleted the std-collections-part1 branch July 16, 2021 01:21

kt3k mentioned this pull request Jul 27, 2021

[Umbrella] std/collections #1065

Closed

26 tasks

ry mentioned this pull request Jul 30, 2021

temporarily remove collections in order to correct author attribution #1079

Closed

ry pushed a commit that referenced this pull request Jul 30, 2021

Add std/collections (#993)

69f3231

Co-authored-by: William Perron <hey@wperron.io>

ry pushed a commit that referenced this pull request Jul 30, 2021

Add std/collections (#993)

0fcd92a

Co-authored-by: William Perron <hey@wperron.io>

LionC mentioned this pull request Aug 27, 2021

[Umbrella] collections APIs #1173

Closed

44 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: std/collections - first part #993

Proposal: std/collections - first part #993

kt3k commented Jun 30, 2021 •

edited

ry left a comment

wperron commented Jul 7, 2021 •

edited

LionC commented Jul 9, 2021

Zzzen Jul 9, 2021

LionC Jul 12, 2021

LionC commented Jul 14, 2021

sebastienfilion commented Jul 14, 2021

LionC commented Jul 14, 2021 •

edited

getspooky commented Jul 14, 2021

LionC commented Jul 14, 2021 •

edited

wperron left a comment

wperron Jul 15, 2021

LionC Jul 15, 2021

wperron Jul 15, 2021

timreichen Aug 5, 2021

wperron Jul 15, 2021

LionC Jul 15, 2021 •

edited

wperron Jul 15, 2021

ry left a comment

Proposal: std/collections - first part #993

Proposal: std/collections - first part #993

Conversation

kt3k commented Jun 30, 2021 • edited

ry left a comment

Choose a reason for hiding this comment

wperron commented Jul 7, 2021 • edited

LionC commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LionC commented Jul 14, 2021

sebastienfilion commented Jul 14, 2021

LionC commented Jul 14, 2021 • edited

getspooky commented Jul 14, 2021

LionC commented Jul 14, 2021 • edited

wperron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LionC Jul 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

kt3k commented Jun 30, 2021 •

edited

wperron commented Jul 7, 2021 •

edited

LionC commented Jul 14, 2021 •

edited

LionC commented Jul 14, 2021 •

edited

LionC Jul 15, 2021 •

edited