bpo-30152: Reduce the number of imports for argparse. by serhiy-storchaka · Pull Request #1269 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| @@ -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)) |
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.
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?
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?
| if type(items) is list: |
|---|
| return items[:] |
| import copy |
| items = copy.copy(items) |
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)' ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
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)?
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__.
| # 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[:] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not items.copy()?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is shorter.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
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.
| import locale |
|---|
| import os |
| import re |
| import struct |
| import sys |
| from errno import ENOENT |
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.
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).
Got rid of importing errno.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| setattr(namespace, self.dest, new_count) |
|---|
| count = getattr(namespace, self.dest, None) |
| if count is None: |
| count = 0 |
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?
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).
eli-schwartz added a commit to eli-schwartz/cpython that referenced this pull request
…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
…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
…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
