-
Notifications
You must be signed in to change notification settings - Fork 64
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 third Parameter $others to farthest and closest Methods #422
Conversation
…e getters typehint" This reverts commit a73f7a3.
I think splitting up an array to 3 parameters would be awkward. Maybe we should support a single variable length parameter. |
Thank you for your comments, but I have a different perspective that I'd like to share. Adding a third parameter, the primary intent of the function remains clear: comparing at least two parameters. IMHO, a single variable-length parameter (array) has disadvantages:
|
I'm not sure we need to require 2 items in the array and named parameters don't mean anything in this case. |
Since PHP 8, when a developer uses named parameters, a single name change (in params) makes method unusable. It's a BC issue. |
This looks good. Users will have to unroll an array anyway and this works with that solution and is BC. |
@othercorey, to support a single variable-length parameter while maintaining backward compatibility, it requires the addition of some additional methods. if this is the path forward, I can make the necessary changes. public function farthest(Chronos $first, Chronos $second, Chronos ...$others): Chronos
{
return $this->findFarthestOrFail([$first, $second, ...$others]);
}
/**
* Find the farthest date from the instance.
* @param iterable<\Cake\Chronos\Chronos> $values
* @return ?self
*/
public function findFarthest(iterable $values): ?Chronos
{
$farthest = null;
$farthestDiff = null;
foreach ($values as $value) {
$otherDiff = $this->diffInSeconds($value);
if (!$farthest || $otherDiff > $farthestDiff) {
$farthest = $value;
$farthestDiff = $otherDiff;
}
}
return $farthest;
}
/**
* Find the farthest date from the instance or fail
* @param iterable<\Cake\Chronos\Chronos> $values
* @return self
* @throws \InvalidArgumentException
*/
public function findFarthestOrFail(iterable $values): Chronos
{
return $this->findFarthest($values) ?? throw new InvalidArgumentException(
'It\'s not possible to find the farthest value as the collection is empty'
);
} |
Currently, the farthest and closest methods in the Chronos class accept only two Chronos objects as parameters, limiting their functionality. There are use cases where it would be beneficial to calculate the farthest or closest Chronos object among three or more.
Proposal:
This pull request aims to enhance the flexibility of the farthest and closest methods by adding a third parameter named $others. This parameter will accept an arbitrary number of Chronos objects, allowing users to calculate the farthest or closest object among multiple instances.
Changes Made:
...$others
to thefarthest
andclosest
methods in theChronos
andChronosDate
classes.This modification maintains backward compatibility, as the methods can still be used with only two parameters if needed. The code has been tested to ensure proper functionality.