bpo-30152: Reduce the number of imports for argparse. by serhiy-storchaka · Pull Request #1269 · python/cpython · GitHub

https://bugs.python.org/issue30152

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

I don't think this change is reasonable. You're micro-optimizing the expense of writing normal maintainable code.

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

…ructor.

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?

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

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?

…rse-reduce-import

Good idea. Thanks @wm75.

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

Oh, right!

A few questions.

Why not isinstance(items, list)?

list subclass can override __copy__ or __reduce__.

Why not items.copy()?

It is shorter.

And it's faster.

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

functools.reduce no longer imported.

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.

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

…rse-reduce-import

Got rid of importing errno.

LGTM

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

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

…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

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

rhettinger reviewed Apr 26, 2017

View reviewed changes

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

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

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

wm75 reviewed May 4, 2017

View reviewed changes

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

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

methane approved these changes Sep 25, 2017

View reviewed changes

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

Mariatta removed the awaiting merge label Oct 8, 2017

eli-schwartz mentioned this pull request Jan 16, 2025

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

Merged

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

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

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

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

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

               import re
-              import struct
               import sys
               from errno import ENOENT

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

bpo-30152: Reduce the number of imports for argparse. by serhiy-storchaka · Pull Request #1269 · python/cpython · GitHub

Uh oh!

Uh oh!

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

bpo-30152: Reduce the number of imports for argparse.#1269serhiy-storchaka merged 11 commits intopython:masterpython/cpython:masterfrom serhiy-storchaka:argparse-reduce-importserhiy-storchaka/cpython:argparse-reduce-importCopy head branch name to clipboard

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

bpo-30152: Reduce the number of imports for argparse.#1269
serhiy-storchaka merged 11 commits into
python:masterfrom
serhiy-storchaka:argparse-reduce-import

serhiy-storchaka commented Apr 24, 2017 •
edited by bedevere-bot
Loading