-
-
Notifications
You must be signed in to change notification settings - Fork 258
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 timeoutWithDefault function #245
Conversation
lib/functions.php
Outdated
|
||
$promise->onResolve(function ($exception, $value) use ($deferred, $default) { | ||
if ($exception) { | ||
$deferred->resolve($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.
An exception should probably be thrown here instead of resulting in the default value.
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, give me a minute.
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 wouldn't make use of the timeout function, but rather adapt the code of it to suit this function.
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.
No wait, that's the whole purpose of the default value. You timeout with a default and when you'll receive a timeout exception, you'll get the default value.
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'll get the default on a timeout, but not if the promise fails?
@kelunik updated |
This is way less code: function timeoutWithDefault($promise, int $timeout, $default = null): Promise
{
return call(function () use ($promise, $timeout, $default) {
try {
return yield timeout($promise, $timeout);
} catch (TimeoutException $excetpion) {
return $default;
}
});
} This is so little code that I question the need for this, but also so little that I don't really care maintance-wise either. |
I think mostly because you originally didn't use You can define functions in your code base you know. 😉 But yes, I can see where this can be useful for many people, so let's add it. |
}); | ||
} | ||
|
||
public function testNonPromise() |
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.
@trowski I tested your suggested implementation and then this test fails, because the TypeError is not thrown.
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 should be…
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.
Oh, right, because it's not resolving the promise with the TypeError. Fine, then it needs to be this way:
function timeoutWithDefault($promise, int $timeout, $default = null): Promise
{
$promise = timeout($promise, $timeout);
return call(function () use ($promise, $default) {
try {
return yield $promise;
} catch (TimeoutException $exception) {
return $default;
}
});
}
lib/functions.php
Outdated
return call(function () use ($promise, $default) { | ||
try { | ||
return yield $promise; | ||
} catch (TimeoutException $excetpion) { |
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.
Totally my fault, but typo: $excetpion 😄
The style fixer is going to yell at you. 🙄 |
done |
see #244