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-38192: Fix remaining passing of "loop" in the protocol examples #16202

Merged
merged 3 commits into from Sep 17, 2019

Conversation

hniksic
Copy link
Contributor

@hniksic hniksic commented Sep 16, 2019

Copy link
Member

@aeros aeros left a comment

Thanks for the PR @hniksic.

Since this is just removing the explicit passing of loop which was is going to be receiving deprecation warnings in 3.8 for several parts of the public API, this looks correct for the most part. The consensus from the asyncio experts seems to be that there is no need for loop to be explicitly passed.

The only suggestion I would make is to modify the ordering within main() (line 949) and removing the comment where the future is created.
Current:

async def main():
    # Get a reference to the event loop as we plan to use
    # low-level APIs.
    loop = asyncio.get_running_loop()

    # Create a pair of connected sockets
    rsock, wsock = socket.socketpair()

    # Prepare a future to inform us when the connection is closed.
    on_con_lost = loop.create_future()

Suggested:

async def main():
    # Get a reference to the event loop as we plan to use
    # low-level APIs.
    loop = asyncio.get_running_loop()
    on_con_lost = loop.create_future()

    # Create a pair of connected sockets
    rsock, wsock = socket.socketpair()

It makes more sense to set up on_con_lost = loop.create_future() right after acquiring a reference to the event loop, rather than after creating the pair of sockets. Even if it provides the same effect functionally, this makes the coroutine's structuring more logical.

The removal of the comment might be a bit more subjective, but personally I don't think it provides additional insight. Especially since it wasn't seen as necessary when it was used in the previous two examples, I don't think it's needed for this one either.

But, we can wait for feedback from @asvetlov and @1st1 before moving forward with the suggested changes.

@hniksic
Copy link
Contributor Author

hniksic commented Sep 17, 2019

@aeros167 Thanks for the review. I like your suggestion and have updated the PR.

@hniksic hniksic changed the title bpo-38192: Fix remaining passing of "loop" in the docs bpo-38192: Fix remaining passing of "loop" in the protocol examples Sep 17, 2019
Copy link
Contributor

@asvetlov asvetlov left a comment

LGTM, thanks!

@asvetlov asvetlov added the 🤖 automerge PR will be merged once it's been approved and all CI passed label Sep 17, 2019
@miss-islington miss-islington merged commit 5d359cc into python:master Sep 17, 2019
@miss-islington
Copy link
Contributor

miss-islington commented Sep 17, 2019

Thanks @hniksic for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒🤖

@bedevere-bot
Copy link

bedevere-bot commented Sep 17, 2019

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

@bedevere-bot
Copy link

bedevere-bot commented Sep 17, 2019

GH-16219 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 17, 2019
miss-islington added a commit that referenced this pull request Sep 17, 2019
miss-islington added a commit that referenced this pull request Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir expert-asyncio 🤖 automerge PR will be merged once it's been approved and all CI passed skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants