-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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-30166: Import command-line parsing modules only when needed. #1293
bpo-30166: Import command-line parsing modules only when needed. #1293
Conversation
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbkaiser, @terryjreedy and @tim-one to be potential reviewers. |
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'm not familiar with IDLE, but rest of the PR looks good to me. Just left a comment about code styling.
Thanks!
@@ -303,6 +302,7 @@ def interact(banner=None, readfunc=None, local=None, exitmsg=None): | |||
|
|||
|
|||
if __name__ == "__main__": | |||
import argparse |
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.
Style nit: You may want to add a new line after import argparse
like the other cases.
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.
This is a thing that compelled me to hesitate. Some usages of argparse have an empty line after imports, some doesn't have (and the majority doesn't have). I added a new line if the following code is large or separated on sections by empty lines, and didn't add it if the following code is short and dense, as in this case.
I left reviewing IDLE on @terryjreedy. |
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.
Moving imports not needed in the main body is consistent with what I did in Fall 2015. It is also how I introduced a bug. So I checked that os can be moved in profile.py and verified the changes in idlelib/pyshell.py. (They will be helpful if I ever move main() to a different module.) The one requested change is obviously not crucial.
Lib/idlelib/pyshell.py
Outdated
@@ -31,14 +30,13 @@ | |||
import tokenize | |||
import warnings | |||
|
|||
from idlelib import testing # bool value | |||
import idlelib |
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.
Move this also to main. 'idlelib' by itself is never used in main body of module.
@@ -25,10 +25,8 @@ | |||
|
|||
|
|||
import sys | |||
import os |
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.
def trace_dispatch (line 184 in 3.6.1) is preceded by an apparently out-of-date comment
# Heavily optimized dispatch routine for os.times() timer
That os.times is no longer used is why the os import can be moved.
self.timer is now time.process time
I have backported the idlelib.pyshell part to 3.6 |
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.
Bahasa indonesia
@ibanezz1999 Please explain your comment. This was merged months ago and any additional changes will be a new patch. |
No description provided.