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

[8.x] Add reduce with keys to collections and lazy collections #35839

Merged
merged 4 commits into from Jan 12, 2021

Conversation

mokhosh
Copy link
Contributor

@mokhosh mokhosh commented Jan 10, 2021

This adds the method reduceWithKeys to Collections and LazyCollections.
Similar to map and mapWithKeys, this augments reduce to pass associative arrays' keys to its callback. We can't do that currently because php's array_reduce doesn't pass array keys to its callback.

So, you can do this:

$data = collect([
    'name' => 'Mo Khosh',
    'username' => 'mokhosh',
]);

return $data->reduceWithKeys(function($carry, $value, $key) {
    return $carry . $key . ': ' . $value . PHP_EOL;
});

I tried to mirror what's already been done with map, mapWithKeys, reduce, and respective tests.

@mokhosh mokhosh changed the title Add reduce with keys to collections and lazy collections [8.x] Add reduce with keys to collections and lazy collections Jan 10, 2021
@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 13, 2021

Thank you @taylorotwell
Added this to docs as well laravel/docs#6740

@JosephSilber
Copy link
Member

The naming here is quite unfortunate 😢

The WithKeys part of mapWithKeys does not mean that it passes the keys to the callback (the regular map already does that). It means that your callback also returns the keys for the new collection.

With this new reduceWithKeys method, the WithKeys part simply says that on each iteration the key will be passed in, but doesn't change anything about the callback's return value.


I would've much preferred not to even have this separate method. When do you ever need the initial value??

Would be much better to simply make this a breaking change in the next release, and have the regular reduce pass 4 arguments: $carry, $value, $key, $initial. That way, if you really need initial you can still use it.

taylorotwell added a commit that referenced this pull request Jan 13, 2021
@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 13, 2021

I think initial is passed at the same level with the closure, it's not passed to the closure.

What do you say we just add $key to the reduce method and use the new implementation instead of array_reduce.
It's not even a breaking change. It just adds a new argument that'll be ignored if you don't use it.

@JosephSilber
Copy link
Member

Oh, I hadn't even checked, and took your word for it. I checked now, and $initial is indeed not being passed to the callback. Was wondering why you would ever want that!

So of course, we should just add it to reduce and be done with it 👍


BTW, this method can go into EnumeratesValues. No need to duplicate the code.

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 14, 2021

One difference I see between some Collection and LazyCollection iterations is we iterate over $this->items in Collections and over $this in LazyCollections.
Do you think we can add this to EnumeratesValues regardless?

@JosephSilber
Copy link
Member

Yes. You can always just iterate over $this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants