-
-
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
[PHPStan Baseline] Fix Errors Part 2 + Modify Unfold
Operation
#217
Conversation
|
||
if ('object' !== $type) { | ||
return $type; | ||
if (!is_object($variable)) { |
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.
#AutoFacepalm
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.
turns out the previous comparison is not interpreted by PHPStan in the same way that the dedicated function comparison is
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.
Great finding!
src/Operation/Unfold.php
Outdated
*/ | ||
static fn (...$parameters): Closure => | ||
/** | ||
* @param callable(mixed|T...): (mixed|array<TKey, T>) $callback | ||
* @param callable(T...): (T|list<T>) $callback |
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 don't understand this change.
$callback
is a user callback, so, it might return anything... no?
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.
well, yes and no.
It's a bit tricky to type this one properly to be honest, so it's up to us if we want to put any restrictions on it or not.
From what I see, this is the behaviour:
- the callback can return any value, let's call it "x"
- in the next call, "x" is passed to the callback as parameter, with the following behaviour:
- if "x" is a list, it's unpacked and each item in the list will be a different parameter for the callback
- if "x" is not a list, then it means it will be the parameter for the callback, and the callback only takes one argument
Examples of each:
$fibonacci = static fn ($a = 0, $b = 1): array =>[$b, $a + $b];
Collection::unfold($fibonacci);
$even = static fn (int $carry): int => $carry + 2;
Collection::unfold($even, -2);
The change I made here means that we are linking the return type of the callback with the parameters that are passed in, but it does also mean that the parameters should be the same type. So we're saying that you can either return a list of parameters of the same type OR a single value which should be the same type as the callback param type.
I think this is fine for most use cases. However, if we want to allow more flexibility the only other option is to go full mixed:
@param callable(mixed ...): mixed $callback
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 agree. The return type of the closure must be a list in order to fulfill the operation correctly.
Maybe we should then get rid of the (array)
cast and array_values
then ? 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.
yeah those are not needed I think, I'll test it
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 removed both now and introduced logic to check whether the $parameters
is a list or a single value. Works the same way as before but makes the code more explicit than the (array)
cast + array_values
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.
Ok, I'll make the needed changes then. It will be a minor backwards compatibility break and will need to update the docs too
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 it might be better to do it now.
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.
yeah I'll do it as part of this PR
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've updated the code and unit tests, have a look how the usage changes. If you're happy with this I can then go ahead and update the documentation and add some static analysis checks
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.
yes definitely :)
src/Operation/Unfold.php
Outdated
@@ -11,6 +11,7 @@ | |||
|
|||
use Closure; | |||
use Generator; | |||
|
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.
😆
*/ | ||
static function () use ($parameters, $callback): Generator { | ||
while (true) { | ||
/** @var T $parameters */ | ||
$parameters = $callback(...array_values((array) $parameters)); | ||
$parameters = $callback(...$parameters); |
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 we should not create a local variable and yield
directly the result of the callback.
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.
that won't work the same way because the Generator uses the variable in the next yield iteration here:
$callback(...$parameters)
If there's no variable then it will continue to use the initial parameters
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.
#TripleFacepalm ... damn I'm too tired!
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.
#TripleFacepalm ... damn I'm too tired!
Don't worry about it 😄. I tried it as well haha, had the same idea initially
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 wait for this to be done, then do the release, if you need some help, just let me know. Tomorrow we're off in Belgium, I might do this tomorrow. |
I'm not sure if I'll get this done today but we should definitely include it in the release, so I'll tag you when it's ready 😉 |
Unfold
Operation
$collection = Collection::unfold($collatz, 10) | ||
->unwrap() | ||
->normalize() | ||
->until(static fn ($number): bool => 1 === $number); |
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 could simplify this as:
$collection = Collection::unfold($collatz, 10)
->unwrap()
->until(static fn (int $number): bool => 1 === $number);
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.
right, that works now because all
normalizes by 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.
👍
Another example | ||
|
||
.. code-block:: php | ||
|
||
$even = Collection::unfold(static function ($carry) {return $carry + 2;}, -2); | ||
$odd = Collection::unfold(static function ($carry) {return $carry + 2;}, -1); | ||
// Is the same as | ||
$even = Collection::range(0, \INF, 2); | ||
$odd = Collection::range(1, \INF, 2); | ||
|
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.
these were repeated in both the range
section and the unfold
section. I kept it for the latter
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.
Fair enough.
@drupol this should be ready now :) |
}; | ||
|
||
$this::unfold($collatz, 10) | ||
->unwrap() | ||
->normalize() |
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 we can get rid of normalize
here as well.
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.
you're right although not without updating the test. I made the update now
docs/pages/api.rst
Outdated
return 1 === $number; | ||
}); | ||
->unwrap() | ||
->normalize() |
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 normalize
can be removed I think.
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 fix the tests :)
All good for you @AlexandruGG ? I can merge it ? |
This PR:
Unfold
operation to make it more consistent in usageFollows #216.