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

serhiy-storchaka · 2017-04-24T07:35:36Z

https://bugs.python.org/issue30152

mention-bot · 2017-04-24T07:35:40Z

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

rhettinger · 2017-04-26T06:46:40Z

Lib/collections/__init__.py

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


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

nedbat · 2017-04-27T18:50:01Z

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.

wm75 · 2017-04-28T15:12:13Z

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 · 2017-04-28T15:31:58Z

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 · 2017-05-03T13:50:01Z

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

serhiy-storchaka · 2017-05-04T05:40:22Z

Good idea. Thanks @wm75.

wm75 · 2017-05-04T07:00:46Z

Lib/argparse.py

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


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

vstinner

A few questions.

vstinner · 2017-09-23T22:59:58Z

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:


Why not isinstance(items, list)?

list subclass can override __copy__ or __reduce__.

vstinner · 2017-09-23T23:00:25Z

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


Why not items.copy()?

It is shorter.

And it's faster.

vstinner · 2017-09-23T23:02:54Z

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


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

functools.reduce no longer imported.

vstinner · 2017-09-23T23:05:17Z

Lib/gettext.py

 import re
-import struct
 import sys
 from errno import ENOENT


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

serhiy-storchaka · 2017-09-24T07:59:16Z

Got rid of importing errno.

methane

LGTM

JimJJewett · 2017-10-03T01:41:26Z

Lib/argparse.py

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


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

Uh oh!

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

serhiy-storchaka commented Apr 24, 2017 •

edited by bedevere-bot

Loading