-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
[3.x] Add template types #60
base: 3.x
Are you sure you want to change the base?
Conversation
🏰 Composer Production Dependency changes 🏰
|
assertType('null', $pool->run(static function (): void { | ||
sleep(1); | ||
})); | ||
|
||
assertType('null', $pool->run(static function (int $time): void { | ||
sleep($time); | ||
}, [1])); |
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.
As mentioned on Discord, a void
method does not produce a result, therefore you must test this by verifying the expression before it is executed:
-assertType('null', $pool->run(static function (): void {
+assertType('Closure(): void', (fn () => $pool->run(static function (): void {
sleep(1);
-}));
+})));
See https://phpstan.org/r/6d97e370-676c-4ce0-b27a-c4eabe6ed4cd
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.
@Ocramius Correct, and normally that's what it is. In this case, if I pass function (): void {}
to https://www.php.net/manual/en/parallel.run.php#refsect1-parallel.run-returnvalues I get a null
back because it's a fire and forget thread. In the other situation when I do return something from a callable I get a https://www.php.net/manual/en/class.parallel-future.php back. Now the return type hinting for that part is all sorted and works as expected. But the void
to null
"translation" would have been nice. I'll put in your suggestions as I don't care too much about this specific test. There is another one I do care about and I think/thought it might have the same root cause, was hoping to point it out but just found out PHPStan didn't run this run for some weird reason. Will let you know once I've fixed 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.
A void
future can still be retrieved, I suppose: it's just that its value can never be assigned to a LHS expression :-)
26e8853
to
e34831f
Compare
e34831f
to
80228d3
Compare
return $runtime->run( | ||
$callable, | ||
$args, | ||
); |
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.
@Ocramius So this line has the actual interesting/problematic error on it and this is also why having it confirmed in the type testing would have been sweet.
Method ReactParallel\Pool\Infinite\Infinite::run() should return T|null but returns T|void|null.
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.
Where is the assertion to check, if that's a test?
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.
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... already commented with my recommendations there? D:
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, and those recommendations there are good, but trying to solve this part of the void issue here.
No description provided.