-
-
Notifications
You must be signed in to change notification settings - Fork 941
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
Support for closing filelike response stream objects #961
Conversation
Current coverage is 100% (diff: 100%)@@ master #961 diff @@
====================================
Files 31 31
Lines 2066 2081 +15
Methods 0 0
Messages 0 0
Branches 335 336 +1
====================================
+ Hits 2066 2081 +15
Misses 0 0
Partials 0 0
|
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 looks great! Just needs a couple quick changes and this is ready to merge.
|
||
|
||
class CloseableStreamIterator(six.Iterator): | ||
""" |
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.
Nitpick: Would you mind reformatting this according to PEP-257?
Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.
E.g.:
class CloseableStreamIterator(six.Iterator):
"""Short summary line ending in a period.
Detailed description.
"""
Also, it may be helpful to mention why a "closeable" iterator is necessary.
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.
Sure, will do.
close_called = False | ||
|
||
def close(self): | ||
super().close() |
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 just needs a quick fix to make it compatible with py26/27.
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 catch, thanks.
Hi @hozn, just following up to see if you'd be able to work on this soon? Hoping to get a new Falcon release out and it would be great to include this patch. Thanks! |
Yeah, sorry, I'll take a look here and see if I can wrap this up. |
Hopefully the updated doc string is more helpful. |
Thanks! |
Described more completely in #960 .