bpo-30152: Reduce the number of imports for argparse. #1269
Conversation
|
@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. |
Lib/collections/__init__.py
Outdated
| @@ -551,7 +550,8 @@ def most_common(self, n=None): | |||
| # Emulate Bag.sortedByCount from Smalltalk | |||
| if n is None: | |||
| 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)) | |||
There was a problem hiding this comment.
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). |
|
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? |
|
Good idea. Thanks @wm75. |
Lib/argparse.py
Outdated
| if type(items) is list: | ||
| return items[:] | ||
| import copy | ||
| items = copy.copy(items) |
There was a problem hiding this comment.
shouldn't that be 'return 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: |
There was a problem hiding this comment.
Why not isinstance(items, list)?
There was a problem hiding this comment.
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[:] |
| inverted = self.__class__(0) | ||
| for m in self.__class__: | ||
| if m not in members and not (m._value_ & self._value_): | ||
| inverted = inverted | m |
There was a problem hiding this comment.
I don't see how this change is related to removing imports.
There was a problem hiding this comment.
functools.reduce no longer imported.
Lib/gettext.py
Outdated
| import locale | ||
| import os | ||
| import re | ||
| import struct | ||
| import sys | ||
| from errno import ENOENT |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is imported by os (where it is only used in one case too).
|
Got rid of importing |
| setattr(namespace, self.dest, new_count) | ||
| count = getattr(namespace, self.dest, None) | ||
| if count is None: | ||
| count = 0 |
There was a problem hiding this comment.
Why not just use getattr(namespace, self.dest, 0) and not have the if test?
There was a problem hiding this comment.
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
…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
…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
https://bugs.python.org/issue30152