Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Grouping via iterator methods? #51

Closed
rauschma opened this issue Jan 4, 2023 · 11 comments
Closed

Grouping via iterator methods? #51

rauschma opened this issue Jan 4, 2023 · 11 comments

Comments

@rauschma
Copy link

rauschma commented Jan 4, 2023

Given how the input is processed, I’m wondering if the grouping operations wouldn’t make more sense as iterator methods. Then they could be used for non-Arrays too.

@zloirock
Copy link
Contributor

zloirock commented Jan 4, 2023

Static methods option already works with iterables / iterators.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2023

It's still very important to be able to use arrays without ever touching the iterator protocol, but I completely agree you should be able to group iterators too.

@jridgewell
Copy link
Member

I mention this a bit in https://github.com/tc39/notes/blob/main/meetings/2022-11/nov-29.md#array-grouping-webcompat-issue:~:text=JRL%3A%20Because%20iterables,contiguous%20runs%20collated: The behavior between grouping a fix size array and an iterable might be a bit different. Iterables can be infinite, and you obviously can't fully consume it to perform the collating step where all the values that produced the same keys are joined into a single array.

// fixed-length array grouping
[1, 2, 2, 1, 2, 2].groupBy(identity);
// => { 1: [1, 1], 2: [2, 2, 2, 2] }

function* nums() {
  while (true) {
    yield * [1, 2, 2];
  }
}
[...nums.groupBy(identity).take(4)];
// => [ [1, [1]], [2, [2, 2]], [1, [1]], [2, [2, 2]] ]

That is, fixed-length grouping always collates the entire collection, but iterable grouping only collates contiguous runs of the same key. The first 1 grouping only has a single element (because the next element returns a different key), the 2 has 2 elements (because there was a contiguous run of 2 keys), followed by a 1 grouping with a single element…

@bakkot
Copy link
Contributor

bakkot commented Jan 5, 2023

To second that, there's established convention from e.g. python and haskell that a groupBy method intended for working with potentially-infinite sequences should group contiguous runs rather than producing a map. I wouldn't want to have an Iterator.prototype.groupBy method that works so differently from Python's itertools.groupby.

But if we do end up with static methods which take iterables, as seems likely, I think it's fine for those methods to assume the iterables are finite - a hypothetical Object.groupBy is more analogous to Object.fromEntries (which also assumes finite iterables) than Iterator.prototype.filter (which does not).

@rauschma
Copy link
Author

rauschma commented Jan 5, 2023

That looks like a different (but also useful) operation to me: “lazy grouping”.

Non-lazy grouping would only work with finite iterators, loosely similar to .reduce().

@zloirock
Copy link
Contributor

zloirock commented Jan 5, 2023

I think that even in the case of Iterator.prototype methods, grouping should return an object or a map, iterator.groupBy(fn) ==== [...iterator].groupBy(fn). Like Iterator#toArray or Iterator#reduce methods that iterate over all iterator instead of returning a wrapper. “Lazy grouping” is something different and not related to this proposal and this issue - but, sure, useful in the future.

@ljharb
Copy link
Member

ljharb commented May 18, 2023

#44 (comment)

The proposal has been demoted to stage 2, with the expectation that I will be requesting stage 3 for it in July with static methods (#47).

Note that these static methods take an iterable/iterator.

@ljharb ljharb closed this as completed May 18, 2023
@rauschma
Copy link
Author

Why not turn them into iterator methods then? Doesn’t Array.group() send the wrong signal?

I’ve been using Rust a lot and I’ve come to appreciate that almost everything is an iterator method (even .map(), .filter() and .find()).

@bakkot
Copy link
Contributor

bakkot commented May 18, 2023

@rauschma These are static methods on Object and Map, not on Array.

As to why not iterator methods, #51 (comment). That's also how e.g. Rust's iterator groupBy works. And it's just a different operation than the one proposed here.

@rauschma
Copy link
Author

rauschma commented May 18, 2023

These are static methods on Object and Map, not on Array.

OK, that works. Static factory methods.

If the only complaint is about the term “group” – why not pick a different, better name (even for the static methods)? There are many precedents of iterator methods only working for finite iterators and returning values (not iterators).

(I’m OK with static factory methods and just wanted to point out alternatives.)

@bakkot
Copy link
Contributor

bakkot commented May 18, 2023

If #51 (comment) is about the term “group” – why not pick a different, better name (even for the static methods)?

The static methods will be named groupBy, which is the best name IMO - at least, it's the name the most people will know.

We definitely could have picked some other, less obvious names and then added these on iterator helpers, but personally I like static methods called "groupBy" better than that option.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants