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-30152: Reduce the number of imports for argparse. #1269

Merged
merged 11 commits into from Sep 25, 2017

Conversation

@serhiy-storchaka
Member

serhiy-storchaka commented Apr 24, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 24, 2017

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @bitdancer and @akheron to be potential reviewers.

mention-bot commented Apr 24, 2017

@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @bitdancer and @akheron to be potential reviewers.

@nedbat

This comment has been minimized.

Show comment
Hide comment
@nedbat

nedbat Apr 27, 2017

Member

People read the stdlib expecting to find best practices. As the discussion on bpo makes clear, these changes are controversial. Could we at least have some comments explaining the unusual lines of code (imports inside functions).

Member

nedbat commented Apr 27, 2017

People read the stdlib expecting to find best practices. As the discussion on bpo makes clear, these changes are controversial. Could we at least have some comments explaining the unusual lines of code (imports inside functions).

@wm75

This comment has been minimized.

Show comment
Hide comment
@wm75

wm75 Apr 28, 2017

Contributor

Regarding the import of copy, I'm not quite sure why it is used at all in the AppendAction/AppendConstAction. Couldn't it be replaced with a simple slice copy of the lists?

Contributor

wm75 commented Apr 28, 2017

Regarding the import of copy, I'm not quite sure why it is used at all in the AppendAction/AppendConstAction. Couldn't it be replaced with a simple slice copy of the lists?

@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka Apr 28, 2017

Member

The default value can be not a list. It can be a deque or a custom sequence with special append method (see an example in https://bugs.python.org/issue16399).

Member

serhiy-storchaka commented Apr 28, 2017

The default value can be not a list. It can be a deque or a custom sequence with special append method (see an example in https://bugs.python.org/issue16399).

@wm75

This comment has been minimized.

Show comment
Hide comment
@wm75

wm75 May 3, 2017

Contributor

I see, thanks. What may be possible though is to make the import of copy even more conditional by first trying a slice copy (which I guess would succeed in > 90% of cases), and resort to copy.copy only if that raises. Maybe needing copy.copy only in very exotic cases makes the unconventional inside-a-function import a bit more justifiable?

Contributor

wm75 commented May 3, 2017

I see, thanks. What may be possible though is to make the import of copy even more conditional by first trying a slice copy (which I guess would succeed in > 90% of cases), and resort to copy.copy only if that raises. Maybe needing copy.copy only in very exotic cases makes the unconventional inside-a-function import a bit more justifiable?

@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka May 4, 2017

Member

Good idea. Thanks @wm75.

Member

serhiy-storchaka commented May 4, 2017

Good idea. Thanks @wm75.

Show outdated Hide outdated Lib/argparse.py
@vstinner

A few questions.

# The copy module is used only in the 'append' and 'append_const'
# actions, and it is needed only when the default value isn't a list.
# Delay its import for speeding up the common case.
if type(items) is list:

This comment has been minimized.

@vstinner

vstinner Sep 23, 2017

Member

Why not isinstance(items, list)?

@vstinner

vstinner Sep 23, 2017

Member

Why not isinstance(items, list)?

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Sep 23, 2017

Member

list subclass can override __copy__ or __reduce__.

@serhiy-storchaka

serhiy-storchaka Sep 23, 2017

Member

list subclass can override __copy__ or __reduce__.

# actions, and it is needed only when the default value isn't a list.
# Delay its import for speeding up the common case.
if type(items) is list:
return items[:]

This comment has been minimized.

@vstinner

vstinner Sep 23, 2017

Member

Why not items.copy()?

@vstinner

vstinner Sep 23, 2017

Member

Why not items.copy()?

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Sep 23, 2017

Member

It is shorter.

@serhiy-storchaka

serhiy-storchaka Sep 23, 2017

Member

It is shorter.

This comment has been minimized.

@rhettinger

rhettinger Sep 24, 2017

Collaborator

And it's faster.

@rhettinger

rhettinger Sep 24, 2017

Collaborator

And it's faster.

inverted = self.__class__(0)
for m in self.__class__:
if m not in members and not (m._value_ & self._value_):
inverted = inverted | m

This comment has been minimized.

@vstinner

vstinner Sep 23, 2017

Member

I don't see how this change is related to removing imports.

@vstinner

vstinner Sep 23, 2017

Member

I don't see how this change is related to removing imports.

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Sep 23, 2017

Member

functools.reduce no longer imported.

@serhiy-storchaka

serhiy-storchaka Sep 23, 2017

Member

functools.reduce no longer imported.

Show outdated Hide outdated Lib/gettext.py
@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka Sep 24, 2017

Member

Got rid of importing errno.

Member

serhiy-storchaka commented Sep 24, 2017

Got rid of importing errno.

@methane

LGTM

@serhiy-storchaka serhiy-storchaka merged commit 8110837 into python:master Sep 25, 2017

4 checks passed

bedevere/issue-number Issue number 30152 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:argparse-reduce-import branch Sep 25, 2017

daxlab added a commit to daxlab/cpython that referenced this pull request Oct 1, 2017

setattr(namespace, self.dest, new_count)
count = getattr(namespace, self.dest, None)
if count is None:
count = 0

This comment has been minimized.

@JimJJewett

JimJJewett Oct 3, 2017

Why not just use getattr(namespace, self.dest, 0) and not have the if test?

@JimJJewett

JimJJewett Oct 3, 2017

Why not just use getattr(namespace, self.dest, 0) and not have the if test?

This comment has been minimized.

@merwok

merwok Oct 3, 2017

Contributor

If think the current code is equivalent to getattr(namespace, self.dest) or 0, not getattr(namespace, self.dest, 0). The first one can never have None for count, the second does (if namespace has an attribute with the name in self.dest and the value None).

@merwok

merwok Oct 3, 2017

Contributor

If think the current code is equivalent to getattr(namespace, self.dest) or 0, not getattr(namespace, self.dest, 0). The first one can never have None for count, the second does (if namespace has an attribute with the name in self.dest and the value None).

@Mariatta Mariatta removed the awaiting merge label Oct 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment