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

bpo-32391: Implement StreamWriter.wait_closed() #5281

Merged
merged 14 commits into from Jan 24, 2018

Conversation

Projects
None yet
4 participants
@asvetlov
Contributor

asvetlov commented Jan 23, 2018

@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 Jan 23, 2018

Member

Please go ahead and add the tests.

Member

1st1 commented Jan 23, 2018

Please go ahead and add the tests.

@asvetlov

This comment has been minimized.

Show comment
Hide comment
@asvetlov

asvetlov Jan 24, 2018

Contributor

Done, please review

Contributor

asvetlov commented Jan 24, 2018

Done, please review

@@ -303,6 +315,13 @@ def can_write_eof(self):
def close(self):
return self._transport.close()
def is_closing(self):

This comment has been minimized.

@1st1

1st1 Jan 24, 2018

Member

Is this a new API method? Please document it if it is.

@1st1

1st1 Jan 24, 2018

Member

Is this a new API method? Please document it if it is.

This comment has been minimized.

@asvetlov

asvetlov Jan 24, 2018

Contributor

Forgot about the change - but exposing the method makes sense

@asvetlov

asvetlov Jan 24, 2018

Contributor

Forgot about the change - but exposing the method makes sense

This comment has been minimized.

@1st1

1st1 Jan 24, 2018

Member

I think it's a bit too low-level. Can you imagine a good use case for async/await code that uses writer and reader?

@1st1

1st1 Jan 24, 2018

Member

I think it's a bit too low-level. Can you imagine a good use case for async/await code that uses writer and reader?

This comment has been minimized.

@asvetlov

asvetlov Jan 24, 2018

Contributor

StreamWriter exposes all Transport methods except flow control related.
is_closing() is not about flow control, I see no reason to not implement it.

@asvetlov

asvetlov Jan 24, 2018

Contributor

StreamWriter exposes all Transport methods except flow control related.
is_closing() is not about flow control, I see no reason to not implement it.

This comment has been minimized.

@1st1

1st1 Jan 24, 2018

Member

OK, go ahead.

@1st1

1st1 Jan 24, 2018

Member

OK, go ahead.

asvetlov added some commits Jan 24, 2018

@asvetlov

This comment has been minimized.

Show comment
Hide comment
@asvetlov

asvetlov Jan 24, 2018

Contributor

Please review again.

Contributor

asvetlov commented Jan 24, 2018

Please review again.

@@ -0,0 +1 @@
Implement :meth:`asyncio.StreamWriter.wait_closed` and :meth:`asyncio.StreamWriter.is_closing` methods

This comment has been minimized.

@1st1

1st1 Jan 24, 2018

Member

One last note: I don't think you want to use ReST formatting in NEWS entries. This would read better:

Implement wait_closed() and is_closing() methods for StreamWriter.
@1st1

1st1 Jan 24, 2018

Member

One last note: I don't think you want to use ReST formatting in NEWS entries. This would read better:

Implement wait_closed() and is_closing() methods for StreamWriter.

This comment has been minimized.

@asvetlov

asvetlov Jan 24, 2018

Contributor

There is no standard for blurb text, the format of messages is mixed.
Some people use ReST, others prefer plain text.
If you briefly look on these files you'll see both cases.

I have no personal preference.

@asvetlov

asvetlov Jan 24, 2018

Contributor

There is no standard for blurb text, the format of messages is mixed.
Some people use ReST, others prefer plain text.
If you briefly look on these files you'll see both cases.

I have no personal preference.

This comment has been minimized.

@1st1

1st1 Jan 24, 2018

Member

I don't really care.

@1st1

1st1 Jan 24, 2018

Member

I don't really care.

@1st1

1st1 approved these changes Jan 24, 2018

@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 Jan 24, 2018

Member

LGTM.

Member

1st1 commented Jan 24, 2018

LGTM.

@asvetlov asvetlov merged commit fe133aa into python:master Jan 24, 2018

4 checks passed

bedevere/issue-number Issue number 32391 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asvetlov asvetlov deleted the asvetlov:asyncio-streams branch Jan 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment