-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Resp prepare #525
Resp prepare #525
Conversation
@@ -629,10 +634,22 @@ def start(coding): | |||
return | |||
|
|||
def start(self, request): | |||
warnings.warn('use .prepare(request) instead', DeprecationWarning) |
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.
Seems .start
remain plain old function while .prepare
is coroutine. Are they really interchangeable during deprecation period?
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.
They are do interchangeable with a note: .prepare()
will process signal handing for response sending but .start()
will not.
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.
Good to know.
I do not like prepare coroutine, right now there is no real use case for coroutine signal. And database example is very good way to shoot yourself in the foot. You should realize, if you give users a gun they for sure shoot themselfs and they will blame you. Python is not fast, and adding more to hot execution path making aiohttp slower. But you are free to add this change, just be cearfull I do not want end up in situation like 0.12 and 0.15 |
The change is backward compatible (all existing code will continue working but for new code In practice users will
I'm not finding the way is elegant. |
Finally ready for merging. |
Signals support is very important for
aiohttp.web
, I see strong request for the feature.We have a PR for that #439 (@alexsdutton, thanks a lot BTW for your work)
I insist signal handlers should be coroutines. Just signal callbacks are very subtle: we may want to access, say, database for modifying response headers (the main purpose for signals right now).
Keeping both synchronous and asynchronous signals just makes a mess -- signals should be coroutines. Period.
Thus I propose deprecation
StreamResponse.start(requiest)
method in favor ofStreamResponse.prepare(request)
coroutine (.begin()
is an option but I feel.prepare()
is better name).The change is backward compatible with limitation: old code still may use
resp.start(request)
way but we will not apply signal handling for it.New code should use
.prepare()
with benefit of signal handling.Any objections? @fafhrd91 @kxepal
TDB:
resp.start()
and catch a deprecation warning). We should do it at least for keeping our test coverage level..start()
deprecated and forcing to brand new.prepare()
usage.