-
Notifications
You must be signed in to change notification settings - Fork 50
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
head/first aligned with lodash/underscore #31
base: master
Are you sure you want to change the base?
Conversation
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.
Nice work! Some initial feedback, I haven't gone through everything yet
@@ -1,5 +1,5 @@ | |||
{ | |||
"name": "lodash-php/lodash-php", | |||
"name": "sfinktah/lodash-php", |
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.
All the changes in this file should be reverted
function headOr(mixed $array, mixed $default) | ||
{ | ||
if ((is_array($array) || $array instanceof \ArrayObject) && count($array)) { | ||
return reset($array); |
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.
Calling reset()
on an ArrayObject
does not work since PHP 7.4, so this needs to be handled separately.
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.
Your many comments are appreciated and inarguably valid. I will not waste time responding to the others, but this one is quite interesting.
Personally, I have always disliked using reset()
to get the head of an array. Coming from C++ it feels like mutating a const ref. I realise array
is passed by value, so no mutation is occuring, though ArrayObject
would be by reference.
I would much rather write
function head($array) {
foreach ($array as $value) {
return $value;
}
return null;
}
function headOr($array, $default) {
head($array) ?? (is_callable($default) ? $default($array) : $default);
}
As you suggested in another comment, headOr
really doesn't need to be overly complex.
That does upset my OCD a little though:
$array = [null, 1, 2];
printf("First: %s\n", head($array, "Empty Array"));
Would return "Empty Array", would it not?
* | ||
* @example | ||
* <code> | ||
* head([1, 2, 3]) |
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.
The code example should reference headOr
*/ | ||
function headOr(mixed $array, mixed $default) | ||
{ | ||
if ((is_array($array) || $array instanceof \ArrayObject) && count($array)) { |
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 think most of this logic can go to the head
function, then this can be simplified to the following:
function headOr($array, $default)
{
return head($array) ?? (is_callable($default) ? $default($array) : $default);
}
* // => null | ||
* </code> | ||
*/ | ||
function headOr(mixed $array, mixed $default) |
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.
The mixed
type is only available since PHP 8, while this library still supports PHP 7.x
{ | ||
return head($array); | ||
} | ||
|
||
$COMMENT = <<<JSON |
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.
This can move to the docblock of the head
function
*/ | ||
function castArray($value): array | ||
{ | ||
$check = function ($value): array { |
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.
This can be simplified to the following:
function castArray($value): array
{
return (array) $value;
}
first (head) was leagues away from being compatible with underscore or lodash.
i did update the tests, but couldn't check them (composer require tree hell with php 8.1)