Added support for ReactPHP Promise v3#53
Conversation
clue
left a comment
There was a problem hiding this comment.
@davidcole1340 Thanks for looking into this! I like the direction this is heading and only added some minor remarks below 👍
src/functions.php
Outdated
| if ($promise instanceof CancellablePromiseInterface) { | ||
| $promise->cancel(); | ||
| if ($promise instanceof PromiseInterface) { | ||
| Promise\resolve($promise)->cancel(); |
There was a problem hiding this comment.
Interesting! I see why this was done, but it looks like static analysis complains about this because resolve() has a PromiseInterface return type for Promise v2 and Promise v1.
Perhaps use logic similar to https://github.com/reactphp/promise-timer/pull/37/files#diff-fe65dcdace9cc44252b537bee79dd574edd1bccf6cee646cc860006a6ec50e8bR15? /cc @WyriHaximus
What do you think about this?
There was a problem hiding this comment.
Perhaps use logic similar to https://github.com/reactphp/promise-timer/pull/37/files#diff-fe65dcdace9cc44252b537bee79dd574edd1bccf6cee646cc860006a6ec50e8bR15? /cc @WyriHaximus
That is the preferred way, making it more explicit, and more performant.
There was a problem hiding this comment.
Yeah I thought so, had this initially but changed it due to this comment. I'll change it back as it's more lightweight.
tests/FunctionAwaitAllTest.php
Outdated
| } | ||
|
|
||
| public function testAwaitAllRejectedWithFalseWillWrapInUnexpectedValueException() | ||
| public function testAwaitAllRejectedWithExceptionWillWrapInUnexpectedValueException() |
There was a problem hiding this comment.
It looks like the name should not have been changed as the logic didn't actually change?
There was a problem hiding this comment.
Oops - accident!
tests/FunctionAwaitAllTest.php
Outdated
| public function testAwaitAllRejectedWithFalseWillWrapInUnexpectedValueException() | ||
| public function testAwaitAllRejectedWithExceptionWillWrapInUnexpectedValueException() | ||
| { | ||
| $this->skipForPromise3(); |
There was a problem hiding this comment.
I see why you've decided to add a helper method for this, but I wonder if it could be more descriptive if we added a reason each time? What do you think? See also all following occurrences.
There was a problem hiding this comment.
Added a reason parameter for the function to pass through when skipping the test
tests/FunctionAwaitAnyTest.php
Outdated
| { | ||
| $all = array( | ||
| $this->createPromiseRejected(1), | ||
| $this->createPromiseRejected(new \Exception(1)), |
There was a problem hiding this comment.
Minor nit only: First argument is a string instead of an int. See also all following occurrences.
There was a problem hiding this comment.
Assuming you mean about the first argument of Exception taking a string? I've changed these to strings now.
|
Thanks for all the work on this PR so far @davidcole1340. We have a new version for |
Unit tests are passing, can test by changing
react/promiseversion todev-master at 2.9.9.