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] Allow collection partition by key and add preserving keys option #16644

Merged
merged 3 commits into from
Dec 6, 2016

Conversation

sileence
Copy link
Contributor

@sileence sileence commented Dec 3, 2016

Some small improvements to the partition method.

  • You can partition a collection by key (typically a column with a boolean value like: published/draft, premium/free, featured/not featured, etc.). i.e. $posts->partition('featured').

  • I also added the preserve keys option similar to the groupBy method.

@sileence sileence force-pushed the collection_partition_by_key branch from 5eec2c8 to 539d044 Compare December 3, 2016 22:13
* @return array
*/
public function partition(callable $callback)
public function partition($callback, $preserveKeys = false)

Choose a reason for hiding this comment

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

I would suggest preserving keys by default. It'll be more natural IMO.

Choose a reason for hiding this comment

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

Well, groupBy doesn't preverse keys by default 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Collections operations generally should preserve keys by default and there should be no option not to as you can call array_values or ->values() if you don't want to.

* @return array
*/
public function partition(callable $callback)
public function partition($callback, $preserveKeys = false)
{
$partitions = [new static(), new static()];
Copy link
Member

Choose a reason for hiding this comment

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

BTW, while you're at it you should probably change this to return a collection, for easy chaining if necessary.

A collection method should really never return an array.

Copy link

@CristianLlanos CristianLlanos Dec 4, 2016

Choose a reason for hiding this comment

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

@JosephSilber, I agree. However, Taylor Otwell made a point about returning an array instead of a Collection object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree since this is a simplify version of the groupBy method and it's meant to be used with list.

Copy link
Member

Choose a reason for hiding this comment

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

You can destructure a collection with list no problem.

Choose a reason for hiding this comment

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

Cool!

@taylorotwell
Copy link
Member

Remove preserveKeys option and make preserving keys the default please.

@sileence sileence force-pushed the collection_partition_by_key branch from 539d044 to 6cc3068 Compare December 5, 2016 10:21
@sileence
Copy link
Contributor Author

sileence commented Dec 5, 2016

@taylorotwell I'm returning a collection because as @JosephSilber pointed out it works with list (the tests pass).

{
$partitions = [new static(), new static()];
$partitions = new static([new static(), new static()]);
Copy link
Member

Choose a reason for hiding this comment

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

  1. We usually leave off the () for constructors with no arguments.
  2. It'd be better to keep $partitions as a simple array, and only wrap it in a collection before returning. It's a minute performance improvement, but hey, why not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the PR

@sileence sileence force-pushed the collection_partition_by_key branch from 8c3c3b6 to 6fb8796 Compare December 5, 2016 15:06
@taylorotwell
Copy link
Member

Can we make this work with the higher order stuff?

@taylorotwell taylorotwell merged commit 61e40c9 into laravel:5.3 Dec 6, 2016
@sileence
Copy link
Contributor Author

sileence commented Dec 6, 2016

@taylorotwell example?

@taylorotwell
Copy link
Member

$collection->partition->free ... I'll look into it.

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