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.

Sign up for GitHub

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

Jump to bottom

Remove explicit "loop" argument from EchoClientProtocol example #82359

Closed
hniksic mannequin opened this issue Sep 15, 2019 · 10 comments
Closed

Remove explicit "loop" argument from EchoClientProtocol example #82359

hniksic mannequin opened this issue Sep 15, 2019 · 10 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes docs Documentation in the Doc dir topic-asyncio

Comments

@hniksic
Copy link
Mannequin

hniksic mannequin commented Sep 15, 2019

BPO 38178
Nosy @rhettinger, @vstinner, @hniksic, @asvetlov, @1st1, @miss-islington
PRs
  • bpo-38178: Don't explicitly pass "loop" to EchoClientProtocol. #16159
  • [3.8] bpo-38178: Don't explicitly pass "loop" to EchoClientProtocol. (GH-16159) #16161
  • [3.7] bpo-38178: Don't explicitly pass "loop" to EchoClientProtocol. (GH-16159) #16162
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2019-09-15.17:23:47.251>
created_at = <Date 2019-09-15.15:17:34.072>
labels = ['3.8', 'docs', '3.7', '3.9', 'expert-asyncio']
title = 'Remove explicit "loop" argument from EchoClientProtocol example'
updated_at = <Date 2019-09-16.08:15:20.338>
user = 'https://github.com/hniksic'

    bugs.python.org fields:

    activity = <Date 2019-09-16.08:15:20.338>
actor = 'vstinner'
assignee = 'docs@python'
closed = True
closed_date = <Date 2019-09-15.17:23:47.251>
closer = 'asvetlov'
components = ['Documentation', 'asyncio']
creation = <Date 2019-09-15.15:17:34.072>
creator = 'hniksic'
dependencies = []
files = []
hgrepos = []
issue_num = 38178
keywords = ['patch']
message_count = 10.0
messages = ['352478', '352480', '352482', '352483', '352484', '352485', '352487', '352488', '352491', '352529']
nosy_count = 7.0
nosy_names = ['rhettinger', 'vstinner', 'hniksic', 'asvetlov', 'docs@python', 'yselivanov', 'miss-islington']
pr_nums = ['16159', '16161', '16162']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue38178'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']
    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Sep 15, 2019

    The EchoClientProtocol example receives a "loop" argument, which is not used at all in the TCP example, and is used to create a future in the UDP example. In modern asyncio code the explicit loop arguments are no longer used since the loop can always be obtained with get_running_loop().

    The proposed patch makes the UDP example consistent with the TCP one (by having the constructor accept the on_con_lost future) and removes the loop argument from both.

    @hniksic hniksic mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes labels Sep 15, 2019
    @hniksic hniksic mannequin assigned docspython Sep 15, 2019
    @hniksic hniksic mannequin added docs Documentation in the Doc dir topic-asyncio labels Sep 15, 2019
    @rhettinger
    Copy link
    Contributor

    The was added originally by Victor and Yuri in c7edffd and 7c7605f

    @miss-islington
    Copy link
    Contributor

    New changeset c717c73 by Miss Islington (bot) (Hrvoje Nikšić) in branch 'master':
    bpo-38178: Don't explicitly pass "loop" to EchoClientProtocol. (GH-16159)
    c717c73

    @miss-islington
    Copy link
    Contributor

    New changeset 701c488 by Miss Islington (bot) in branch '3.7':
    bpo-38178: Don't explicitly pass "loop" to EchoClientProtocol. (GH-16159)
    701c488

    @rhettinger
    Copy link
    Contributor

    Hrvoje, thanks for fixing this.

    Next time, please involve the people who originally wrote the code. They are both still active. If they made mistakes, it helps them to know about it. If they had a specific intent for the code example, it allows them to make sure the objectives are still being achieved.

    @miss-islington
    Copy link
    Contributor

    New changeset 1cd6e92 by Miss Islington (bot) in branch '3.8':
    bpo-38178: Don't explicitly pass "loop" to EchoClientProtocol. (GH-16159)
    1cd6e92

    @asvetlov
    Copy link
    Contributor

    Thanks Hrvoje!

    @hniksic
    Copy link
    Mannequin Author

    hniksic mannequin commented Sep 15, 2019

    Raymond, no problem; I guess I assumed that the authors are following the bug tracker (or have possibly moved on and are inactive).

    I also had reason to believe the change to be non-controversial, since it is in line with Yury's own recommendations, e.g. from his 2018 EuroPython presentation (slide 14):

    https://speakerdeck.com/1st1/asyncio-today-and-tomorrow

    In any case, I am happy that the pull request has been processed and accepted so quickly - thanks everyone for the great work!

    @asvetlov
    Copy link
    Contributor

    Raymond, sorry if I was so quick in applying.
    The patch is very trivial and obvious.
    I am pretty sure that Yuri and Victor approve it.

    The PR doesn't fix a mistake, the code is still valid.
    But we all changed our mind what is the loop role, should it be explicitly passed everywhere or not, etc.

    At the time of the example creation explicit loop was safer, after Python 3.5.3 bugfx release the implicit loop became the idiomatic solution.

    We use the implicit loop everywhere, 3.8 raises DeprecationWarning for passing the loop into certain API calls.
    The PR just changes the example to follow our own recommendations.

    @vstinner
    Copy link
    Member

    In modern asyncio code the explicit loop arguments are no longer used since the loop can always be obtained with get_running_loop().

    Yeah, the trend changed. Around Python 3.4, passing explicitly loop was preferred for best performances.

    Since that time, the code to get the current loop has been optimized, and the new trend is to make the loop implicit to make the code more readable.

    --

    When I wrote the doc, self.loop.stop() was called explicitly:

    https://docs.python.org/3.5/library/asyncio-protocol.html#tcp-echo-client-protocol

    It seems like the example has been modified to add a new "on_con_lost" Future.

    --

    Anyway, thanks Hrvoje Nikšić for your contribution ;-)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes docs Documentation in the Doc dir topic-asyncio
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @asvetlov @rhettinger @miss-islington