GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-30152: Reduce the number of imports for argparse. by serhiy-storchaka · Pull Request #1269 · python/cpython

Conversation

serhiy-storchaka commented Apr 24, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

https://bugs.python.org/issue30152

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

yan12125 and wm75 reacted with thumbs up emoji

All reactions

👍 2 reactions


          bpo-30152: Reduce the number of imports for argparse.

eccaf69

serhiy-storchaka added the type-feature A feature request or enhancement label

Apr 24, 2017

the-knights-who-say-ni added the CLA signed label

Apr 24, 2017

mention-bot commented Apr 24, 2017

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rhettinger reviewed

Apr 26, 2017

View reviewed changes

Comment thread

Lib/collections/__init__.py Outdated

		return sorted(self.items(), key=_itemgetter(1), reverse=True)
		return _heapq.nlargest(n, self.items(), key=_itemgetter(1))
		import heapq
		return heapq.nlargest(n, self.items(), key=_itemgetter(1))

rhettinger Apr 26, 2017

Copy link

Copy Markdown

Contributor

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 don't think this change is reasonable. You're micro-optimizing the expense of writing normal maintainable code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka added 2 commits

April 26, 2017 23:48


          Revert changes in collections.

79b085f


          Revert movint an argparse import. This will be done in separate PR.

30c87cd

serhiy-storchaka closed this

Apr 27, 2017

serhiy-storchaka reopened this

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

serhiy-storchaka and louisom reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 2 commits

April 28, 2017 08:08


          Revert moving import gettext. gettext is used in ArgumentParser const…

46c77d3

…ructor.


          Add the comments explaining imports inside functions.

d8a622c

wm75 commented Apr 28, 2017

Copy link

Copy Markdown

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka commented Apr 28, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

wm75 commented May 3, 2017

Copy link

Copy Markdown

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka added 2 commits

May 4, 2017 08:20


          Merge branch 'master' of https://github.com/python/cpython into argpa…

dad5bae

…rse-reduce-import


          Import copy only when items is not a list.

578af90

serhiy-storchaka commented May 4, 2017

Copy link

Copy Markdown

Member Author

Good idea. Thanks @wm75.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

wm75 reviewed

May 4, 2017

View reviewed changes

Comment thread

Lib/argparse.py Outdated

		if type(items) is list:
		return items[:]
		import copy
		items = copy.copy(items)

wm75 May 4, 2017

Copy link

Copy Markdown

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.

shouldn't that be 'return copy.copy(items)' ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka May 4, 2017

Copy link

Copy Markdown

Member Author

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.

Oh, right!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix a bug in _copy_items().

48a2abf

yan12125 mentioned this pull request

Jun 17, 2017

bpo-29442: Replace optparse with argparse in setup.py #139

Merged

rhettinger approved these changes

Sep 23, 2017

View reviewed changes

bedevere-bot added the awaiting merge label

Sep 23, 2017

vstinner reviewed

Sep 23, 2017

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

A few questions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/argparse.py

		# 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:

vstinner Sep 23, 2017

Copy link

Copy Markdown

Member

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.

Why not isinstance(items, list)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 23, 2017

Copy link

Copy Markdown

Member Author

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.

list subclass can override __copy__ or __reduce__.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/argparse.py

		# 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[:]

vstinner Sep 23, 2017

Copy link

Copy Markdown

Member

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.

Why not items.copy()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 23, 2017

Copy link

Copy Markdown

Member Author

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.

It is shorter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rhettinger Sep 24, 2017

Copy link

Copy Markdown

Contributor

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.

And it's faster.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/enum.py

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

vstinner Sep 23, 2017

Copy link

Copy Markdown

Member

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 don't see how this change is related to removing imports.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 23, 2017

Copy link

Copy Markdown

Member Author

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.

functools.reduce no longer imported.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/gettext.py Outdated

		import re
		import struct
		import sys
		from errno import ENOENT

vstinner Sep 23, 2017

Copy link

Copy Markdown

Member

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.

Would it be worth it to defer errno import as well? It's only used in one case, to raise an error when something goes wrong.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 23, 2017

Copy link

Copy Markdown

Member Author

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.

It is imported by os (where it is only used in one case too).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka added 3 commits

September 24, 2017 02:34


          Merge branch 'master' into argparse-reduce-import

502bf6a


          Merge branch 'master' of https://github.com/python/cpython into argpa…

c449538

…rse-reduce-import


          Get rid of importing errno.

025688d

serhiy-storchaka added the skip news label

Sep 24, 2017

serhiy-storchaka commented Sep 24, 2017

Copy link

Copy Markdown

Member Author

Got rid of importing errno.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

methane approved these changes

Sep 25, 2017

View reviewed changes

methane left a comment

Copy link

Copy Markdown

Member

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka merged commit 8110837 into python:master

Sep 25, 2017

serhiy-storchaka deleted the argparse-reduce-import branch

September 25, 2017 21:55

methane mentioned this pull request

Sep 26, 2017

bpo-31581: Reduce the number of imports for functools #3757

Merged

JimJJewett reviewed

Oct 3, 2017

View reviewed changes

Comment thread

Lib/argparse.py

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

JimJJewett Oct 3, 2017

Copy link

Copy Markdown

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

merwok Oct 3, 2017

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mariatta removed the awaiting merge label

Oct 8, 2017

eli-schwartz added a commit to eli-schwartz/cpython that referenced this pull request

Jan 16, 2025


          pythongh-118761: improve optparse import time by delaying textwrap im…

97f0271

…port

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most
invocations will never need it imported.

See: python#1269
See: 8110837

eli-schwartz added a commit to eli-schwartz/cpython that referenced this pull request

Jan 16, 2025


          pythongh-118761: improve optparse import time by delaying textwrap im…

4484efc

…port

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most
invocations will never need it imported.

See: python#1269
See: 8110837

eli-schwartz mentioned this pull request

Jan 16, 2025

gh-118761: improve optparse import time by delaying textwrap import #128899

Merged

eli-schwartz added a commit to eli-schwartz/cpython that referenced this pull request

Jan 17, 2025


          pythongh-118761: improve optparse import time by delaying textwrap im…

10cd152

…port

The same change was made, and for the same reason, by argparse back in
2017. The textwrap module is only used when printing help text, so most
invocations will never need it imported.

See: python#1269
See: 8110837

Reviewers

vstinner

vstinner left review comments

merwok

merwok left review comments

rhettinger

rhettinger approved these changes

methane

methane approved these changes

+2 more reviewers

wm75

wm75 left review comments

JimJJewett

JimJJewett left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news type-feature

A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

serhiy-storchaka commented Apr 24, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nedbat commented Apr 27, 2017

Uh oh!

wm75 commented Apr 28, 2017

Uh oh!

serhiy-storchaka commented Apr 28, 2017

Uh oh!

wm75 commented May 3, 2017

Uh oh!

serhiy-storchaka commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Sep 24, 2017

Uh oh!

methane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

serhiy-storchaka commented Apr 24, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.