-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix typing for async route methods #614
base: trunk
Are you sure you want to change the base?
Conversation
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 think this file might not be a great idea for testing types; we should have most of this type stuff covered by annotated tests. But in any case I have some comments inline about what these type signatures should be.
src/klein/test/typing_app.py
Outdated
# methods. | ||
|
||
@router.route("/async-object") | ||
async def asyncReturnsObject(self, request: IRequest) -> KleinRenderable: |
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.
-> KleinRenderable
isn't doing what you think here, because KleinRenderable
includes both KleinSynchronousRenderable
and Awaitable[KleinSynchronousRenderable]
, and you can't return both of those here; due to the implicit modification of async def
, the actual return type of this function becomes (handwaving a bit over the Coroutine
junk) Awaitable[Union[KleinSynchronousRenderable, Awaitable[KleinSynchronousRenderable]]
, which is gibberish.
What you actually want to do with these methods is to say, for example, "-> str
", which will implicitly match against -> KleinRenderable
.
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.
OK that works for cases where I know the specific type I'm returning. What if I'm returning the result of something that gives me a KleinRenderable
?
Presumably I will have awaited on it if appropriate, so I agree that maybe this should be KleinSynchronousRenderable
, though that's currently not public.
That is, it should be possible to type this generically.
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.
If I use KleinSynchronousRenderable
, I at least get passing for these, but it doesn't catch asyncReturnsObject
as invalid… so not perfect, but better.
Add some
async
methods totyping_app.py
to verify that those work… and they do not:These errors are a bit tough to read… I gave it a stab but I'm not sure why this is unhappy.