Skip to content
This repository has been archived by the owner. It is now read-only.

/ asyncio Archived

Change import statements to import only what's needed #468

Open
wants to merge 4 commits into
base: master
from
Open

Change import statements to import only what's needed #468

wants to merge 4 commits into from

Conversation

@denisra
Copy link

denisra commented Nov 16, 2016

Changed all the import statements in all modules in the examples directory to import only what is needed.

This fixes issue #464
@denisra
Changed all the import statements in all modules in the examples
Loading status checks…
3863b43 
directory to import only what is needed.

This fix issue #464
@1st1
Copy link
Member

1st1 commented Nov 16, 2016

How about we do simple

import asyncio

at the top of each example? get_event_loop() should become asyncio.get_event_loop().

That's the style we recommend in Python documentation and use in asyncio code itself.
@denisra
Copy link
Author

denisra commented Nov 16, 2016

That makes sense. Let me fix that.
@denisra
Some minor improvements on coding style.
Loading status checks…
5e4efd7 
Fixes issue #464
@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Nov 16, 2016

Hello, and thanks for your contribution!

Unfortunately we couldn't find an account corresponding to your GitHub username at bugs.python.org (b.p.o). If you don't already have an account at b.p.o, please create 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 the PSF 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.
@denisra
Copy link
Author

denisra commented Nov 16, 2016

@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.
@brettcannon
Copy link
Member

brettcannon commented Nov 16, 2016

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.
@brettcannon brettcannon added CLA signed and removed CLA signed labels Nov 16, 2016
@brettcannon
Copy link
Member

brettcannon commented Nov 16, 2016

OK, bot is fixed; sorry about that.
@brettcannon
brettcannon suggested changes Nov 16, 2016
View changes
Copy link
Member

brettcannon left a comment

In terms of imports you need to be careful about side-effect access to submodule that just happen to be imported from asyncio itself, e.g. don't rely on test_utils existing on asyncio.
examples/cacheclt.py Outdated
@@ -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()

This comment has been minimized.

Sign in to view
@brettcannon

brettcannon Nov 16, 2016

Member

Unless it's documented as such, there's no guarantee this will hold in the future. It would be better to import asyncio.test_utils explicitly.
examples/source.py Outdated
@@ -3,8 +3,7 @@
import argparse
import sys

from asyncio import *
from asyncio import test_utils
import asyncio

This comment has been minimized.

Sign in to view
@brettcannon

brettcannon Nov 16, 2016

Member

Also import asyncio.test_utils.
examples/source1.py Outdated
@@ -3,8 +3,8 @@
import argparse
import sys

from asyncio import *
from asyncio import test_utils
import asyncio

This comment has been minimized.

Sign in to view
@brettcannon

brettcannon Nov 16, 2016

Member

asyncio.test_utils

This comment has been minimized.

Sign in to view
@1st1

1st1 Nov 17, 2016

Member

I wonder why we use test_utils anyways. It should be removed from examples.

This comment has been minimized.

Sign in to view
@denisra

denisra Nov 17, 2016

Author

@brettcannon Thanks for pointing that out. I made the changes as suggested. Please let me know if I need to change anything else.
@denisra
Changed how we are importing the test_utils submodule as suggested
Loading status checks…
a40fe76
@denisra
Copy link
Author

denisra commented Nov 17, 2016

@brettcannon I can also confirm that I've signed the CLA.
@1st1 1st1 removed the CLA not signed label Nov 17, 2016
@1st1
Copy link
Member

1st1 commented Nov 17, 2016

@denisra I wonder if we could remove import asyncio.test_utils completely. This is an internal module, and has nothing to do with examples (which historically were just experiments with asyncio, while asyncio was in active development).
@brettcannon
brettcannon approved these changes Nov 17, 2016
View changes
@denisra
Copy link
Author

denisra commented Nov 17, 2016

@1st1 is it ok if I create another issue and submit another PR for that, to keep things separated?
@1st1
Copy link
Member

1st1 commented Nov 17, 2016

@1st1 is it ok if I create another issue and submit another PR for that, to keep things separated?

OK, let's do that!
@1st1
1st1 reviewed Nov 17, 2016
View changes
examples/cacheclt.py Outdated
import json
import logging

import asyncio
import asyncio.test_utils

This comment has been minimized.

Sign in to view
@1st1

1st1 Nov 17, 2016

Member

Imports should be sorted alphabetically (that's what we try to do in asyncio and CPython code bases).

This comment has been minimized.

Sign in to view
@denisra

denisra Nov 18, 2016

Author

@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.

This comment has been minimized.

Sign in to view
@sethmlarson

sethmlarson Nov 18, 2016

@denisra Asyncio is technically a stdlib module. ;)

This comment has been minimized.

Sign in to view
@denisra

denisra Nov 18, 2016

Author

@SethMichaelLarson I can't say that it isn't :) will send send in another commit in just a few min with that fixed
@denisra
Changed the import statements so they are sorted alphabetically
Loading status checks…
b92866f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@1st1 1st1

@sethmlarson sethmlarson

@brettcannon brettcannon

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@denisra @1st1 @the-knights-who-say-ni @brettcannon @sethmlarson
You can’t perform that action at this time.