9972: support yield on coroutines within inlineCallbacks functions#12239
9972: support yield on coroutines within inlineCallbacks functions#12239adiroiban merged 3 commits intotwisted:trunkfrom
Conversation
CodSpeed Performance ReportMerging #12239 will not alter performanceComparing Summary
|
5178a49 to
a002fe7
Compare
|
please review |
a002fe7 to
2ad8ce9
Compare
adiroiban
left a comment
There was a problem hiding this comment.
Thanks. Many thanks for working on this.
Only minor comment.
Please review before the merge.
Thanks again!
| it is possible to rewrite functions using C{inlineCallbacks} to C{async def} | ||
| in piecewise manner and be mostly compatible to existing code. | ||
|
|
||
| The rewrite process is simply replacing C{inlineCallbacks} decorator with |
There was a problem hiding this comment.
Minor comment.
I am not sure if we need this paragraph.
I would say that this info can be part of defer-intro.rst
Otherwise we end up with duplicated documentation.
There was a problem hiding this comment.
I think that docstrings and defer-intro.rst serve different purposes. The former is a reference, a person knows what he wants to do and just comes to look for details. The latter is tutorial - a person does not know how to achieve something, thus one reads tutorial start to end to get general understanding.
In this particular case I think we should mention that there's a better way to do things in both places. I agree that this introduces some duplication, but it serves a practical purpose that developers reading the documentation are less confused. I think this saves time overall if we think about both developers of this project and its users.
Finally, reducing duplication serves the purpose of reducing the chances of errors, when only one repetition is updated. I think this is less of concern in this case, because the next change is likely to be removal of inlineCallbacks some years into the future.
| yield getDivisionFailureCoro("OMG") | ||
| except ZeroDivisionError as e: | ||
| self.assertEqual(str(e), "OMG") | ||
| returnValue("WOOSH") |
There was a problem hiding this comment.
Why not use the standard return statement ?
| returnValue("WOOSH") | |
| return "WOOSH" |
There was a problem hiding this comment.
This tests that returnValue still works. Once we actually remove the feature we can simplify the tests too.
There was a problem hiding this comment.
My worry is that returValue now raises a deprecation warning and these new tests are not asserting or clearing that warning.
This is not a big issue now... but it adds some tech debt.
In an ideal case, we will run each test while capturing any warning and failing if there are unhandled / un-asserted warnings.
Unfortunately, trial has no support for automatic catching of the warnings that are generated during a test run... and this is why the test are not yet failing.
I understand that the current tests have a wider coverage ... so we can keep this code.
I was thinking that since returnValue is now a thing of the past, we can stop worrying about it and don't bother too much with covering returnValue in new tests.
If returnValue don't work with coroutine, that should be fine... as the original code written using returnValue is old code from the time when coroutines were not supported.
This essentially embeds Deferred.fromCoroutine() into within inlineCallbacks(). This allows piecewise porting of inlineCallbacks-based codebases to coroutines. As long as addCallback(), addErrback() and friends are not used on results of inlineCallbacks-decorated functions, the conversion process is very simple.
209e92d to
21e56ca
Compare
|
@adiroiban Thanks for review, I've addressed all of your comments - either with a fix or a comment why I think the fix is not necessary. |
|
Thanks for the update. I have merged this. I left some feedback wheter we should continue to worry about |
Scope and purpose
Fixes #9972.
The change is very simple - it essentially embeds Deferred.fromCoroutine() into within inlineCallbacks().
This allows piecewise porting of inlineCallbacks-based codebases to coroutines. As long as addCallback(), addErrback() and friends are not used on results of inlineCallbacks-decorated functions, the conversion process is very simple.