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] Pass $key to closure in Collection and LazyCollection's reduce method as well #35878

Merged
merged 11 commits into from
Jan 15, 2021

Conversation

mokhosh
Copy link
Contributor

@mokhosh mokhosh commented Jan 14, 2021

Following the discussion we had here #35839 I've removed reduceWithKeys and changed the reduce method to pass $key to its closure as well as $value.

The previous test is there. I've just added another test for closures that accept the key, since I thought you might want to have one assertion per test. If you prefer to have one test per method and you don't care how many assertions are in one test, we can merge the two tests.

Comment on lines 3604 to 3606
$this->assertEquals('foobarbazqux', $data->reduceWithKeys(function ($carry, $element, $key) {
$this->assertEquals('foobarbazqux', $data->reduce(function ($carry, $element, $key) {
return $carry .= $key.$element;
}));
Copy link
Member

@JosephSilber JosephSilber Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Since we're still keeping reduceWithKeys around till 9.x (because it was already tagged), we shouldn't remove its test.
  2. You can just add the $key to the other test. No need for 2 separate tests.
  3. As discussed on that other PR, you can move the reduce method into the EnumeratesValues trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thanks for the comment

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 15, 2021

I think we're there.

  1. kept the test for 8.x and removed it in the pull request I made for 9.x
  2. added test with key to the same test
  3. moved both methods to EnumeratesValues

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 15, 2021

Doesn't StyleCI remove those 4 spaces automatically?

@driesvints driesvints changed the title [8.x] Pass $key to closure in Collection and LazyColletion's reduce method as well [8.x] Pass $key to closure in Collection and LazyCollection's reduce method as well Jan 15, 2021
@driesvints
Copy link
Member

@mokhosh upon merging, not for forks.

@mokhosh
Copy link
Contributor Author

mokhosh commented Jan 15, 2021

We're good then? Or do i fix style manually?

@driesvints
Copy link
Member

@mokhosh it's always better if everything's green of course but StyleCI issues will get fixed automatically upon merging.

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.

4 participants