-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Operation
#152
Add Equals
Operation
#152
Conversation
src/Operation/Equals.php
Outdated
); | ||
|
||
// Point free style. | ||
return $pipe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a work in progress, but here's my thoughts just by reading it quickly:
Be careful! It's a trap !
Using Diff
means that you're going to go through the whole iterator before deciding the outcome.
But yet what you're trying to do is to compare if they are equal, in other words, find if all the elements are equal, in other words: find if there is at least two elements that are different.
It means that as soon as two elements are different, there is no need to go through the whole iterator, we know that they are different.
It's then a job for the MatchOne
or Every
operations (My favorite operations btw!)
Example:
<?php
/**
* For the full copyright and license information, please view
* the LICENSE file that was distributed with this source code.
*/
declare(strict_types=1);
namespace loophp\collection\Operation;
use Closure;
use Generator;
use Iterator;
use MultipleIterator;
/**
* @immutable
*
* @template TKey
* @template T
*/
final class Equals extends AbstractOperation
{
/**
* @pure
*
* @return Closure(Iterator<TKey, T>): Closure(Iterator<TKey, T>): Generator<int, bool>
*/
public function __invoke(): Closure
{
return
/**
* @param Iterator<TKey, T> $other
*
* @return Closure(Iterator<TKey, T>): Generator<int, bool>
*/
static function (Iterator $left): Closure {
return static function (Iterator $right) use ($left): Generator
{
$mit = new MultipleIterator(MultipleIterator::MIT_NEED_ANY);
$mit->attachIterator($left);
$mit->attachIterator($right);
return yield Every::of()
(static fn (): bool => true)
(static fn (array $current): bool => $current[0] !== $current[1]); // Should we allow comparison customization with a callback?
($mit)->current();
};
};
}
}
Or if you want to optimize things:
<?php
/**
* For the full copyright and license information, please view
* the LICENSE file that was distributed with this source code.
*/
declare(strict_types=1);
namespace loophp\collection\Operation;
use Closure;
use Generator;
use Iterator;
use MultipleIterator;
/**
* @immutable
*
* @template TKey
* @template T
*/
final class Equals extends AbstractOperation
{
/**
* @pure
*
* @return Closure(Iterator<TKey, T>): Closure(Iterator<TKey, T>): Generator<int, bool>
*/
public function __invoke(): Closure
{
return
/**
* @param Iterator<TKey, T> $other
*
* @return Closure(Iterator<TKey, T>): Generator<int, bool>
*/
static function (Iterator $left): Closure {
return Pipe::of()(
static function (Iterator $iterator) use ($left): Iterator {
$mit = new MultipleIterator(MultipleIterator::MIT_NEED_ANY);
$mit->attachIterator($left);
$mit->attachIterator($iterator);
return $mit;
},
Every::of()
(static fn (): bool => true)
(static fn (array $current): bool => $current[0] !== $current[1]),
Current::of()(0)
);
};
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @drupol, thanks for this; does this work if the elements are in different positions in the collections?
As in:
Collection (1, 2, 3)
Collection (2, 3, 1)
These should still be considered equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or are you thinking we should consider them unequal?
I feel like there's scope to have 2 operations maybe: equals
and same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @drupol, thanks for this; does this work if the elements are in different positions in the collections?
As in:
Collection (1, 2, 3)
Collection (2, 3, 1)
No it doesn't do that sadly.
I feel like there's scope to have 2 operations maybe:
equals
andsame
Exact, we should have 2 different operations for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, in that case:
equals
would need to check the entire collection, so this implementation should be finesame
should stop as soon as an element is different or the end of one collection is reached, because we want them in the same positions
Regarding allowing the user to customise the comparison behaviour:
I want at least one of the operations (I'm thinking equals
) to only accept a single parameter, otherwise it won't work with PHPUnit's assertObjectEquals
method: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Constraint/Object/ObjectEquals.php#L97-L102
Perhaps for same
we can allow that with the signature:
Collection::same(Collection $other, ?callable $comparator = null): bool
.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems ok to me, I guess we will refine all of these during the review.
Let's not forget to mention in the documentation that there is a "cost" with this operation. |
Thanks! Didn't forget it, I was just debating whether to add it now or when we add the |
By the way, I'm thinking about this optimization: diff --git a/src/Operation/Equals.php b/src/Operation/Equals.php
index d0bae356..6f091110 100644
--- a/src/Operation/Equals.php
+++ b/src/Operation/Equals.php
@@ -50,7 +50,13 @@ final class Equals extends AbstractOperation
return yield false;
}
- return yield from Pipe::of()(Diff::of()(...$other), IsEmpty::of())($iterator);
+ foreach ($iterator as $current) {
+ if (false === Contains::of()($current)($other)->current()) {
+ return yield false;
+ }
+ }
+
+ return yield true;
};
};
} I think this would be better. Indeed, the WDYT ? |
docs/pages/api.rst
Outdated
@@ -721,6 +721,10 @@ Elements will be compared using strict equality (``===``). | |||
.. tip:: This operation enables comparing ``Collection`` objects in PHPUnit tests using | |||
the dedicated `assertObjectEquals`_ assertion. | |||
|
|||
.. warning:: Because this operation *needs to traverse both collections* to determine if | |||
the same elements are contained within them, a performance cost is incurred. It is not | |||
recommended to use this for potentially large collections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 !
We can even use diff --git a/src/Operation/Equals.php b/src/Operation/Equals.php
index d0bae356..52304548 100644
--- a/src/Operation/Equals.php
+++ b/src/Operation/Equals.php
@@ -50,7 +50,12 @@ final class Equals extends AbstractOperation
return yield false;
}
- return yield from Pipe::of()(Diff::of()(...$other), IsEmpty::of())($iterator);
+ return yield from Pipe::of()(
+ Every::of()
+ (static fn(): bool => false)
+ (static fn($current): bool => Contains::of()($current)($other)->current()),
+ Head::of()
+ )($iterator);
};
};
} |
Hmm interesting one! |
This is what the |
I just noticed this after I wrote it 😄 |
This is what I would propose: diff --git a/src/Operation/Equals.php b/src/Operation/Equals.php
index d0bae356..dcbfe2d0 100644
--- a/src/Operation/Equals.php
+++ b/src/Operation/Equals.php
@@ -50,7 +50,10 @@ final class Equals extends AbstractOperation
return yield false;
}
- return yield from Pipe::of()(Diff::of()(...$other), IsEmpty::of())($iterator);
+ return yield from Every::of()
+ (static fn(): bool => false)
+ (static fn($current): bool => Contains::of()($current)($other)->current())
+ ($iterator);
};
};
} And like that, we gracefully get rid of the |
Good for me now! Feel free to merge whenever you want! |
This is great, I used it just had to tweak it a bit to satisfy phpcs and then psalm 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I'll call it a day and give this a fresh final look tomorrow! But feel free to approve and I will merge it if I'm not changing anything. |
@drupol thanks for all the help on this! 💯 |
This PR:
Equals
which allows determining if two collections contain the same elementsIsEmpty
-> now returns aGenerator
so it can be piped. This is backwards-compatible at Collection level but a BC break at individual Operation leveldiff
Related to #135 (comment).