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.

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

Merged
merged 4 commits into from
May 4, 2017

Conversation

serhiy-storchaka
Copy link
Member

No description provided.

@mention-bot
Copy link

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

Copy link
Member

@berkerpeksag berkerpeksag left a 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
Copy link
Member

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.

Copy link
Member Author

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.

@serhiy-storchaka
Copy link
Member Author

I left reviewing IDLE on @terryjreedy.

Copy link
Member

@terryjreedy terryjreedy left a 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.

@@ -31,14 +30,13 @@
import tokenize
import warnings

from idlelib import testing # bool value
import idlelib
Copy link
Member

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
Copy link
Member

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

@serhiy-storchaka serhiy-storchaka merged commit 7e4db2f into python:master May 4, 2017
@serhiy-storchaka serhiy-storchaka deleted the import-argparse branch May 4, 2017 05:17
@terryjreedy
Copy link
Member

terryjreedy commented May 4, 2017

I have backported the idlelib.pyshell part to 3.6

Copy link

@ibanezz1999 ibanezz1999 left a comment

Choose a reason for hiding this comment

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

Bahasa indonesia

@terryjreedy
Copy link
Member

@ibanezz1999 Please explain your comment. This was merged months ago and any additional changes will be a new patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants