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

bpo-38242: Revert "bpo-36889: Merge asyncio streams (GH-13251)" #16482

Merged
merged 1 commit into from
Sep 30, 2019

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Sep 30, 2019

See https://bugs.python.org/issue38242 for more details

99% of the work is done by @aeros167; I've just needed to add a few more fixes and couldn't push to his branch; hence the new PR.

https://bugs.python.org/issue38242

@aeros
Copy link
Contributor

aeros commented Sep 30, 2019

99% of the work is done by @aeros167; I've just needed to add a few more fixes and couldn't push to his branch; hence the new PR.

Thanks for finishing this up Yury. Out of curiosity, what additional changes needed to be made?

@1st1
Copy link
Member Author

1st1 commented Sep 30, 2019

Thanks for finishing this up Yury. Out of curiosity, what additional changes needed to be made?

.write() and .close() were still awaitable -- which technically was committed before new streams api. Still we want to revert streams to their 3.7 state.

.. coroutinefunction:: start_server(client_connected_cb, host=None, \
port=None, \*, loop=None, limit=2**16, \
port=None, \*, loop=None, limit=None, \
Copy link
Contributor

@aeros aeros Sep 30, 2019

Choose a reason for hiding this comment

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

The constant _DEFAULT_LIMIT (2**16) is still being used as the default for limit in start_server(), so this can be restored.

Suggested change
port=None, \*, loop=None, limit=None, \
port=None, \*, loop=None, limit=2**16, \

Copy link
Contributor

@aeros aeros Sep 30, 2019

Choose a reason for hiding this comment

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

I can do this in a separate PR if needed. This change is rather low priority in comparison to reverting the streaming API, and would not be a release blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, although right now docs are the same as in 3.7. So I'm not sure it's needed tbh.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I'm not sure it's needed tbh.

Okay, I won't worry about it then, it's fairly trivial.

@aeros
Copy link
Contributor

aeros commented Sep 30, 2019

.write() and .close() were still awaitable -- which technically was committed before new streams api. Still we want to revert streams to their 3.7 state.

Ah, I wasn't aware we were also removing that change. I hadn't realized we were reverting fully back to the 3.7 API, I thought it was primarily just focused on 23b4b69. Now it makes a lot more sense why you wanted to remove _asyncio_internal.

@bedevere-bot
Copy link

@1st1: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @1st1, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6758e6e12a71ef5530146161881f88df1fa43382 3.8

@bedevere-bot
Copy link

GH-16485 is a backport of this pull request to the 3.8 branch.

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.

5 participants