-
-
Notifications
You must be signed in to change notification settings - Fork 932
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
use an async context manager factory for lifespan #1227
use an async context manager factory for lifespan #1227
Conversation
starlette/routing.py
Outdated
return gen_wrapper | ||
|
||
|
||
def _wrap_lifespan_context( |
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.
this stuff is a bit of a huge mess I think it's better to just make this a breaking change and make people add contextlib.asynccontextmanager
to their functions
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 agree...
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.
@tomchristie are you happy for this to be a breaking change?
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.
Could you explain what would be necessary for a user to convert their existing lifespan generator to an asynccontextmanager
? i.e. an example?
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.
You just add the @contextlib.asynccontextmanager
decorator
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.
@JayH5 thank you so much for this comment! Because of this I worked out I could get rid of all my ugly _wrap_lifespan_context stuff and just use asynccontextmanager(lifespan_context)
Kinda sorta related (at least involves some similar code): #1205 |
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.
Was going to suggest updating the documentation, but now I see that we don't have docs for the generator lifespan stuff. It therefore seems likely to me that most users are using on_startup
/on_shutdown
, so the deprecation shouldn't affect too many.
Am happy with this PR and it's a nice clean up.
elif inspect.isasyncgenfunction(lifespan): | ||
warnings.warn( | ||
"async generator function lifespans are deprecated, " | ||
"use an @contextlib.asynccontextmanager function instead", |
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 now read this like it should be "a contextlib..."
"use an @contextlib.asynccontextmanager function instead", | |
"use a @contextlib.asynccontextmanager function instead", |
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 read this as an at contextlib.asynccontextmanager
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.
Nice 👍
fixes #1138
special thanks to @JayH5 for: https://github.com/encode/starlette/pull/1205/files#diff-aca25e5f16c4fd49338ccdf3631f72455309335fee1e27f3d3b6016fa94ecedfR531-R538