-
Notifications
You must be signed in to change notification settings - Fork 10
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
Refactor Promise classes with generic types #24
Refactor Promise classes with generic types #24
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't V
also need to be declared in the class annotations?
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.
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? |
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.
can you please rebase on the master branch and solve the merge conflict?
and fix the reported cs issues?
@@ -6,5 +6,3 @@ finder: | |||
path: | |||
- "src" | |||
|
|||
enabled: | |||
- short_array_syntax |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries, whenver you have time
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
then
method 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