Skip to content

bpo-37199: Replace the early returns added in c2cda63 #14535

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

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Jul 1, 2019

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Using a conditional for an early return seems to be particularly unconventional across the rest of Python, and can somewhat reduce the readability of the code when it is done too many times, approved.

This is not specifically related to this issue, but what are your thoughts on the usage of the single letter variable names t and p in on lines 1275 and 1282? Changing the variable name t to task for example seems like it would improve the readability, particularly for anyone who is not as intimately familiar with async. I'm not arguing for t to be named task specifically if there's another preferred name. I would be in favor of any name that accurately summarizes the purpose of the variable within the method.

@benjaminp
Copy link
Contributor

merging because it addresses review comments on GH-14480.

@miss-islington
Copy link
Contributor

Thanks @ZackerySpytz for the PR, and @benjaminp for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

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

miss-islington added a commit that referenced this pull request Sep 13, 2019
(cherry picked from commit 81319a8)

Co-authored-by: Zackery Spytz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants