Fix typing of Lifespan to allow subclasses of Starlette #2077
Conversation
tests/test_applications.py
Outdated
| def test_lifespan_typing(): | ||
| class App(Starlette): | ||
| pass | ||
|
|
||
| @asynccontextmanager | ||
| async def lifespan(app: App) -> AsyncIterator[None]: | ||
| yield | ||
|
|
||
| App(lifespan=lifespan) |
There was a problem hiding this comment.
Not sure if we actually enforce mypy on our tests, but pylance reports no issues
There was a problem hiding this comment.
Awesome, thank you @adriangb! This looks good.
I just tested this branch locally to confirm it works, and I made the corresponding PR in FastAPI here, I'll merge that and release once this is merged and released: fastapi/fastapi#9245
starlette/applications.py
Outdated
| @@ -257,3 +257,6 @@ def decorator(func: typing.Callable) -> typing.Callable: | |||
| return func | |||
|
|
|||
| return decorator | |||
|
|
|||
|
|
|||
| AppType = typing.TypeVar("AppType", bound=Starlette) | |||
There was a problem hiding this comment.
Can we move this to the top, or is there an issue with string typing?
There was a problem hiding this comment.
No we can't. It needs to reference Starlette so it has to come after it.
There was a problem hiding this comment.
Just tested here, and it works before...
starlette/types.py
Outdated
| @@ -1,7 +1,6 @@ | |||
| import typing | |||
|
|
|||
| if typing.TYPE_CHECKING: | |||
| from starlette.applications import Starlette | |||
| T = typing.TypeVar("T") | |||
There was a problem hiding this comment.
| T = typing.TypeVar("T") | |
| AppType = typing.TypeVar("AppType") |
|
The lifespan parameter on |
|
Good point. We probably have to add an Any |
| AppType = typing.TypeVar("AppType", bound="Starlette") | ||
|
|
There was a problem hiding this comment.
Does this work? If so 👍🏻
| @@ -11,6 +11,8 @@ | |||
| from starlette.routing import BaseRoute, Router | |||
| from starlette.types import ASGIApp, Lifespan, Receive, Scope, Send | |||
|
|
|||
| AppType = typing.TypeVar("AppType", bound="Starlette") | |||
There was a problem hiding this comment.
Check if you are fine with this, please. @adriangb
Fixes fastapi/fastapi#9241 (comment)