[WIP][2.x] Add template annotations #223
Conversation
WyriHaximus
force-pushed
the
2.x-add-template-annotations
branch
4 times, most recently
from
b15cc8d
to
82b32eb
Compare
WyriHaximus
force-pushed
the
2.x-add-template-annotations
branch
2 times, most recently
from
9ab066b
to
b87eda7
Compare
WyriHaximus
force-pushed
the
2.x-add-template-annotations
branch
from
05d3135
to
4dafbdc
Compare
|
Sorry for barging but I am really interested in this PR and would like to remove my own stubs. Should I wait for merge or use PR branch instead? |
|
@zmitic Honestly same, will have a look at this again during the weekend and hopefully, this got resolved in recent PHPStan updates. But currently support for mixed scalar and scalar through |
|
Opened phpstan/phpstan#8707 to hopefully get the last few functions fully in |
WyriHaximus
force-pushed
the
2.x-add-template-annotations
branch
from
4dafbdc
to
d08f97d
Compare
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
|
PR's to address our current template param buggy use can be found here: |
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
In the previous PR (reactphp#63) I missed incorrect usage in the readme, this PR addresses that. The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
| @@ -2,9 +2,17 @@ | |||
|
|
|||
| namespace React\Promise; | |||
|
|
|||
| /** | |||
| * @template-implements PromiseInterface | |||
| * @template-covariant T | |||
There was a problem hiding this comment.
It would be nice to also have a type parameter for rejection reason. But I have tried doing that for guzzlehttp/promisees and it was a pain.
There was a problem hiding this comment.
Even if it's a pain, does it work? Guess, based on no PR associated with those changes it doesn't?
One of the issues is literally this: guzzle/promises@master...jtojnar:guzzle-promises:generic-types-2#diff-08c91937f94e1c62ad95bd704218d97cff992fb41a9b71b7fa9519442f8d67dbR33 that checks what is passed in to handle both scenarios not what is coming out of the previous promise.
That is one of the reasons we decided not to bother with rejections (for now).
There was a problem hiding this comment.
I made a PR but gave up because the various handler propagations were too difficult to reason about and I ran out of time I allocated for the experiment.
| assertType('React\Promise\PromiseInterface<array<bool|int>>', all([true, time()])); | ||
| assertType('React\Promise\PromiseInterface<array<bool|int>>', all([resolve(true), resolve(time())])); | ||
| assertType('React\Promise\PromiseInterface<array<bool|float>>', all([resolve(true), hrtime()])); | ||
| assertType('React\Promise\PromiseInterface<array<bool|int>>', all([true, resolve(time())])); |
There was a problem hiding this comment.
Perhaps it would be fine to comment these out for now. Most of the time people will be using homogeneous lists anyway. And if someone needs to use a heterogeneous one, they can always add it to ignore list in phpstan.neon.
There was a problem hiding this comment.
Lines 46 and 47 work fine, lines 48 and 49 don't. If the bug in PHPStan doesn't get fixed before this PR will be merged we'll comment 48 and 49 out. As that's not a very common situation. 46 and maybe 47 I have a lot in applications where I don't directly control the typing and accept multiple. An example is the GitHub client I've been generating having a Repository and a SimpleRepository. Both work for where I use them and it gets either depending on where something originates from. So something returning an array with both is perfectly possible.
| @@ -13,8 +13,10 @@ | |||
| * | |||
| * If `$promiseOrValue` is a promise, it will be returned as is. | |||
| * | |||
| * @param mixed $promiseOrValue | |||
| * @return PromiseInterface | |||
There was a problem hiding this comment.
Looks like this actually returns ExtendedPromiseInterface.
There was a problem hiding this comment.
Technically yes, but that isn't the base contract we're using. This is also why in v3 ExtendedPromiseInterface has been merged into PromiseInterface.
WyriHaximus
force-pushed
the
2.x-add-template-annotations
branch
from
d08f97d
to
a640e74
Compare
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
No description provided.