Skip to content
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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Conversation

wsanchez
Copy link
Member

Add some async methods to typing_app.py to verify that those work… and they do not:

src/klein/test/typing_app.py:87:6: error: Value of type variable "KleinRouteHandlerT" of function cannot be
"Callable[[Application, IRequest], Coroutine[Any, Any, Union[Union[Union[str, bytes, IResource, IRenderable, Tag, None], Iterable[Union[str, bytes, IResource, IRenderable, Tag, None]]], Awaitable[Union[Union[str, bytes, IResource, IRenderable, Tag, None], Iterable[Union[str, bytes, IResource, IRenderable, Tag, None]]]]]]]"
 [type-var]

These errors are a bit tough to read… I gave it a stab but I'm not sure why this is unhappy.

@wsanchez wsanchez added the bug label May 24, 2023
@wsanchez wsanchez self-assigned this May 24, 2023
@wsanchez wsanchez requested a review from a team as a code owner May 24, 2023 00:28
Copy link
Member

@glyph glyph left a 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 Show resolved Hide resolved
# methods.

@router.route("/async-object")
async def asyncReturnsObject(self, request: IRequest) -> KleinRenderable:
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants