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

[5.3] Add split method to collection class #15302

Merged
merged 6 commits into from
Sep 6, 2016
Merged

[5.3] Add split method to collection class #15302

merged 6 commits into from
Sep 6, 2016

Conversation

freekmurze
Copy link
Contributor

This PR adds a split method to Illuminate\Support\Collection that splits a collection into the given number of groups.

$collection = collect(['a', 'b', 'c', 'd'])->split(2);

$collection->count(); // returns 2

$collection->first(); // returns a collection with 'a' and 'b';
$collection->last(); // returns a collection with 'c' and 'd';

@@ -921,6 +921,23 @@ public function slice($offset, $length = null)
}

/**
* Split a collection into a certain number of groups.
*
* @param int $numberOfGroups
Copy link
Member

Choose a reason for hiding this comment

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

missing space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Still not correct?

@@ -920,6 +920,23 @@ public function slice($offset, $length = null)
return new static(array_slice($this->items, $offset, $length, true));
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

This whole block is indented by one too many spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honest mistake, fixed now (using the patch file from style ci)

Copy link
Member

Choose a reason for hiding this comment

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

Still incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

StyleCI aligned it to your method definition which is also indented incorrectly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wut? Can StyleCI be incorrect? 😄

Copy link
Member

Choose a reason for hiding this comment

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

StyleCI is correct according to Laravel's rules. The phpdoc must be aligned to match the method. There is no fixer to change the alignment of the actual method definition yet.

/**
* Split a collection into a certain number of groups.
*
* @param int $numberOfGroups
Copy link
Member

Choose a reason for hiding this comment

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

When I said a missing space, I really did mean exactly one space was missing. :trollface:

Please put exactly two spaces inbetween int and $.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, my apologies for all the trouble.

public function split($numberOfGroups)
{
if ($this->isEmpty()) {
return $this;
Copy link
Member

Choose a reason for hiding this comment

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

should return a new instance here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

@arrilot
Copy link
Contributor

arrilot commented Sep 6, 2016

Refs #10889

@taylorotwell taylorotwell merged commit 2bfef8b into laravel:5.3 Sep 6, 2016
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

4 participants