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

[#8088] Support async/await with Deferreds #453

Merged
merged 30 commits into from Aug 3, 2016
Merged

[#8088] Support async/await with Deferreds #453

merged 30 commits into from Aug 3, 2016

Conversation

hawkowl
Copy link
Member

@hawkowl hawkowl commented Aug 1, 2016

@codecov-io
Copy link

codecov-io commented Aug 1, 2016

Current coverage is 89.97% (diff: 100%)

Merging #453 into trunk will decrease coverage by 0.26%

@@              trunk       #453   diff @@
==========================================
  Files           837        838     +1   
  Lines        144857     144783    -74   
  Methods           0          0          
  Messages          0          0          
  Branches      13597      13590     -7   
==========================================
- Hits         130729     130272   -457   
- Misses        11891      12272   +381   
- Partials       2237       2239     +2   

Powered by Codecov. Last update 6c6fea1...5ffe7c0

raise StopIteration(self.result)
return self.d

def __await__(self):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also implement __iter__ (also returning self).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Should this actually need to implement __await__ at all? Since Deferred is the thing with await, I just need to change it to call __iter__ and rename the __await__ on this object...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it should only implement __iter__, and no __await__:

class Deferred:
    def __await__(self):
        return _MakeDeferredAwaitable(self)

class _MakeDeferredAwaitable(object):
    def __iter__(self):
        return self

    def __next__(self):
         if self.result is not _NO_RESULT:
             raise StopIteration(self.result)
         return self.d

Per PEP 492 -- __await__ should return an iterator (not an awaitable). Also, you might want to implement _MakeDeferredAwaitable.throw if it's possible in Twisted to throw exceptions to the await points.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1st1 right, hmm, that's fixable.

re throw: it's not, because the Deferred will fire with a Failure; which inlineCallbacks will then throw into it (https://github.com/twisted/twisted/pull/453/files/5ffe7c0aed2c3dcea26ab6c6af8cb1ac7bf5f216#diff-41cf94828242f95ae48c543d7db3309aR1183).


AwaitTests = _g["AwaitTests"]
__all__ = ["AwaitTests"]
except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive-by comment!

This is too broad, in that weird misconfigurations will raise weird errors and these tests silently won't run.

  1. Move the compat and FilePath imports, and the _path construction, out of the try block. Even the _g = {}. Presumably the only thing that should fail is the execfile, so that's the only thing that should be in the try.
  2. Catch only the exception that's the problem (SyntaxError, I'm guessing?) not bare-word except.
  3. Even the silly obvious can't fail stuff after the execfile should be in the else suite of a try/except/else; you don't want to swallow any more errors than you have to.
  4. Consider adding an AwaitTests class that just has a single, empty test method and a "skip" attribute, so there's at least visibility that the test was there, and it was intentionally not run.
  5. Maybe don't even have a try. This is always supposed to work on 3.5, right? So just compare sys.version_info and do it unconditionally.

This comment brought to you by literally dozens of saturday nights ruined by NameError or AttributeErrorss in some obscure try block that "would only fail in this one case".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess "conditionally", not "unconditionally", but still; what I mean is, "without a try".

@hawkowl hawkowl merged commit 57a2311 into trunk Aug 3, 2016
@tomprince tomprince deleted the await-8088 branch August 4, 2016 18:35
return d


When writing coroutines, you do not need to use :api:`twisted.internet.defer.ensureDeferred <ensureDeferred>` when you are writing a coroutine which calls other coroutines which await on Deferreds; you can just ``await`` on it directly.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is repetition of "when writing"

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

Successfully merging this pull request may close these issues.

None yet

5 participants