DusanKasan/Knapsack

groupBy - Allow group by keys

Closed this issue · 5 comments

Hey,

First of all thanks for the great library.
I was just wondering if we can add an option to groupBy a key in the array. This is more useful when dealing with associated arrays. This feature is there in almost all collection libraries (Laravel, CakePHP, etc)

An example usecase.

$categories = [
   ['id' => 1, 'name' => 'Category 1', 'parent_id' => 0],
   ['id' => 2, 'name' => 'Category 2', 'parent_id' => 1],
   ['id' => 3, 'name' => 'Category 3', 'parent_id' => 1],
];

$grouped = Collection::from($categories)->groupBy('parent_id');

Cheers

Hi,

i can see the benefit of implementing this. But, at the moment only 2 functions from Knapsack have some expectations about the underlying items. These are flatten and pluck. Flatten is fine, but pluck is kind of like the groupBy you are proposing.

What i mean is: the collection functions should not be opinionated about the type of collection items. That's why i'm even considering removing pluck. Because the Collection is lazy, you can not tell it contains items you can't process before you iterate over them and try to access some index on an object or something.

That being said, nested collection traversing is the main usecase here and the problem i described above would still exist if you did groupBy(function ($i) {return $i['key'];)}).

So i guess i will either add groupByKey($key) or change groupBy to groupBy(string|callable $groupBy). I would prefer the latter since callable can also be passed as a string and there could be conflicts. Also i'll need to refactor pluck a little bit to skip non-collection items (like this groupBy will do) and reflect this to the docs.

TL;DR: I'll add the feature in some way as soon as possible. Also you helped me to find a bug in pluck which will also be fixed 👍

Maybe i'll even add a "strict" flag to pluck and groupBy that will throw ItemNotFound when there will be some non-collection items. That would have to realize the items of course so we would know this before iteration.

Cool! I have implemented it in this commit, in case you want to have a look :)

Nice. Seems cool, but as i look at it maybe groupByKey method would be better, so we have the concerns separated and all. You can revert this change and introduce the groupByKey(mixed $key) function+method that would ignore the non-collection items if you want :)

This way we will avoid BC (the callable/key conflicts) and releasing a new major version.

Sure, will do that soon.