Refactor Promise classes with generic types#24
Refactor Promise classes with generic types#24Radiergummi wants to merge 4 commits intophp-http:masterfrom
Conversation
This commit refactors the Promise interface and concrete classes to use generic types. This provides better support for static analysis tools, enforcing type safety and improving code readability.
| * @param callable|null $onFulfilled called when a response will be available | ||
| * @param callable|null $onRejected called when an exception occurs | ||
| * @param callable(T): V|null $onFulfilled called when a response will be available | ||
| * @param callable(\Exception): V|null $onRejected called when an exception occurs |
There was a problem hiding this comment.
doesn't V also need to be declared in the class annotations?
There was a problem hiding this comment.
In that case, V is determined by whatever then returns, which is possibly not related to the promise's original T, so there's an additional @template V on the method comment.
In TypeScript, it would be:
interface Promise<T> {
then<V>(onFulfilled: (value: T) => V | Promise<V>): Promise<V>;
}|
i fixed the cs issue in the master branch, please rebase. the documentation for promise is managed in the documentation repository: https://github.com/php-http/documentation/blob/main/components/promise.rst however we don't talk about implementing your own promises. if you want to add a section at the end titled "Implementing your own Promises" or something, you could explain it there. on the other hand, i think for people used to generics, its kind of self-explanatory. |
👍
Yeah, that's was I was wondering. Let me give it a shot, maybe a single paragraph on generic type support is enough to simply let people know they can use it if they like? |
dbu
left a comment
There was a problem hiding this comment.
can you please rebase on the master branch and solve the merge conflict?
and fix the reported cs issues?
| - "src" | ||
|
|
||
| enabled: | ||
| - short_array_syntax |
There was a problem hiding this comment.
not sure what happened, it looks like instead of rebase you added the commits from master as new commits in the branch. if you can fix it, cool, otherwise i can solve that before merging.
There was a problem hiding this comment.
Sorry, I had some personal stuff coming up last week and didn't get around checking this. Looks like my IDE messed up the rebase, I'm going to fix the issue. Thanks for your patience!
There was a problem hiding this comment.
no worries, whenver you have time
There was a problem hiding this comment.
@Radiergummi thank you so much for this change. Might you be in a position to rebase?
This would also really benefit our SDK Generator
There was a problem hiding this comment.
@dbu thinking of forking @Radiergummi's changes, rebasing and submitting a new PR. Thoughts? Or should we give this more time?
There was a problem hiding this comment.
glad if you want to do that. you can add the changes in a new commit and then the work of each contributor is still properly attributed.
Refactor Promise classes with generic types (fast-tracking #24)
|
wrapped up in #27, thanks @Radiergummi ! |
What's in this PR?
This PR refactors the Promise interface and concrete classes to use generic types. This allows to hold a meta-reference to the type of value the promise will resolve to. The
thenmethod will return a separate template type, so you can actually build properly typed then-chains.For the Promise template, a covariant template type has been introduced. This allows library authors to create their own, constrained Promise types (say,
UserPromise<U extends User>).All in all, these annotations will improve type safety in a lot of code bases.
Regarding the docs
I'm not quite sure whether these additions should be added to the documentation (probably). WDYT?
Example Usage
I'm copying the (contrived) example from #23 here:
Checklist
To Do