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

Add equals method to collection #23544

Closed
wants to merge 1 commit into from
Closed

Add equals method to collection #23544

wants to merge 1 commit into from

Conversation

akoury
Copy link

@akoury akoury commented Mar 14, 2018

Hey, first time contributing to the framework, hope this is not too bad and sorry if it is

Similar to the is() method for model comparison I wanted to add an equals method to compare two collections based on their values.

My need to add this comes from testing that the collection a view received is the same as the one you created in a test, this method would allow a simple check like so:

$orders = factory(Order::class, 3)->create();

$this->get(route('orders.index'))
     ->assertViewHas('orders', function ($viewOrders) use ($orders) {
        return $viewOrders->equals($orders);
     });

If it is a single model you can easily do the comparison with the is() method, however I believe there is nothing similar for collections.

The way I do the check is by using the diff method in collections and verifying that the count equals 0 in both ways (that means that a is equal to b and that b is equal to a, because based on the way diff works it can lead to different results).

Thanks and again, sorry if I missed anything or if my implementation is too naive.

@browner12
Copy link
Contributor

thanks for getting involved!

traditionally Collection methods are not accepted, but we'll see what Taylor says. usually the suggestion is just to macro them into your own project.

if this gets merged, my one concern would be with the naming. when I hear 'equals' I think more along the lines of mathematically. would there be another term that would work?

@decadence
Copy link
Contributor

Just today needed this and was surprised that Laravel doesn't have it

@akoury
Copy link
Author

akoury commented Mar 14, 2018

@browner12 'match' and 'sameAs' come to mind

$viewOrders->match($orders);

$viewOrders->sameAs($orders);

@taylorotwell
Copy link
Member

How does array_diff with objects work under the hood? What is it comparing?

@decadence
Copy link
Contributor

It's comparing string representation of array elements

@MohannadNaj
Copy link
Contributor

Thumbs up for this. but why not array_diff_assoc? it's more reliable since it compares both keys and values, not the values only.

Using diff will evaluate this to true:

 collect([1,2,3])->equals([1,3,1,2])
 collect(['a'=>1,'b'=>2])->equals(collect([1,2]))

Check out this SO post and diffAssoc PR.

@taylorotwell
Copy link
Member

What if they can't be converted to string?

@akoury
Copy link
Author

akoury commented Mar 15, 2018

@MohannadNaj using diffAssoc was my first attempt, however for my precise use case it did not work so I tried with diff and it did work, but based on your examples I believe you are right in that it should use diffAssoc.

I also tried a different approach using intersections and comparing the counts like so:

$givenItems = $this->getArrayableItems($items);

$originalItemsCount = count($this->items);
$givenItemsCount = count($givenItems);
$intersectionCount = $this->intersectByKeys($givenItems)->intersect($givenItems)->count();

if ($intersectionCount === $originalItemsCount && $originalItemsCount === $givenItemsCount) {
    return true;
}

return false;

This approach works for the cases you mentioned and for my specific case, however I do not know how to work with the case @taylorotwell mentioned

Let me know if you want me to push the code I mentioned here or just change diff to diffAssoc

@taylorotwell
Copy link
Member

I think I will just let people use diff directly for 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.

5 participants