Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork178
Change import statements to import only what's needed#468
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
directory to import only what is needed.This fix issuepython#464
How about we do simple importasyncio at the top of each example? That's the style we recommend in Python documentation and use in asyncio code itself. |
That makes sense. Let me fix that. |
the-knights-who-say-ni commentedNov 16, 2016
Hello, and thanks for your contribution! Unfortunately we couldn't find an account corresponding to your GitHub username atbugs.python.org (b.p.o). If you don't already have an account at b.p.o, pleasecreate one and make sure to add your GitHub username. If you do already have an account at b.p.o then please go there and under "Your Details" add your GitHub username. And in case you haven't already, please make sure to sign thePSF contributor agreement (CLA); we can't legally look at your contribution until you have signed the CLA. Once you have done everything that's needed, please reply here and someone will verify everything is in order. |
@1st1 Please let me know if you need me to change anything else. @the-knights-who-say-ni This is my first contribution. I've just created my account at bugs.python.org and added my Github username to it. Please let me know if I missed anything. |
Please ignore any label fiddling I do; you managed to find a bug with the@the-knights-who-say-ni bot. @denisra do make sure to sign the CLA. |
OK, bot is fixed; sorry about that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In terms of imports you need to be careful about side-effect access to submodule that just happen to be imported fromasyncio itself, e.g. don't rely ontest_utils existing onasyncio.
| @@ -172,7 +173,7 @@ def main(): | |||
| loop = asyncio.new_event_loop() | |||
| sslctx = None | |||
| if args.tls: | |||
| sslctx = test_utils.dummy_ssl_context() | |||
| sslctx =asyncio.test_utils.dummy_ssl_context() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unless it's documented as such, there's no guarantee this will hold in the future. It would be better to importasyncio.test_utils explicitly.
| @@ -3,8 +3,7 @@ | |||
| import argparse | |||
| import sys | |||
| from asyncio import * | |||
| from asyncio import test_utils | |||
| import asyncio | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alsoimport asyncio.test_utils.
| @@ -3,8 +3,8 @@ | |||
| import argparse | |||
| import sys | |||
| from asyncio import * | |||
| from asyncio import test_utils | |||
| import asyncio | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
asyncio.test_utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder why we usetest_utils anyways. It should be removed from examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@brettcannon Thanks for pointing that out. I made the changes as suggested. Please let me know if I need to change anything else.
@brettcannon I can also confirm that I've signed the CLA. |
@denisra I wonder if we could remove |
@1st1 is it ok if I create another issue and submit another PR for that, to keep things separated? |
OK, let's do that! |
| import json | ||
| import logging | ||
| import asyncio | ||
| import asyncio.test_utils | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Imports should be sorted alphabetically (that's what we try to do in asyncio and CPython code bases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@1st1 I wasn't aware of that, sorry. I was going by the PEP8 guidelines, where it tells us to import the standard library modules first. But I'll have that fix following your recommendations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@denisra Asyncio is technically a stdlib module. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@SethMichaelLarson I can't say that it isn't :) will send send in another commit in just a few min with that fixed
Changed all the import statements in all modules in the examples directory to import only what is needed.
This fixes issue#464