bpo-32156: Remove unused imports #4614
Conversation
Fix flake8 warning F401, ex: 'newer_pairwise' imported but unused.
Doc/includes/email-headers.py
Outdated
| @@ -1,5 +1,6 @@ | |||
| # Import the email modules we'll need | |||
| from email.parser import BytesParser, Parser | |||
| # from email.parser import BytesParser | |||
There was a problem hiding this comment.
I'd say move this line to line 6 and change the comment to "[...] these three lines".
| @@ -1,10 +1,9 @@ | |||
| #!/usr/bin/env python3 | |||
|
|
|||
| from Cocoa import NSMutableDictionary, NSMutableArray, NSString, NSDate, NSNumber | |||
| from Cocoa import NSPropertyListSerialization, NSPropertyListOpenStepFormat | |||
| from Cocoa import NSPropertyListSerialization | |||
| # from Cocoa import NSPropertyListOpenStepFormat | |||
There was a problem hiding this comment.
In https://bugs.python.org/issue14455#msg190950 Ronald said:
- There is no support for the old OpenStep format. That format is badly
documented, has been deprecated for a long time, and writing it is
not supported by Apple's plist libraries. The OpenStep format also
seems to be much more limited than the two modern ones.
Perhaps we could just delete this and the following line:
# ('openstep', NSPropertyListOpenStepFormat),There was a problem hiding this comment.
When I work on such large "cleanup" patches, I really fear regressions. While your proposition makes sense, IMHO it's out of the scope of the PR. I don't know this code, I don't know how to test it. So I'm too shy to modify. If you mind, please write a separated PR for that.
Here I chose to copy the coding style of the file: keep the code but comment it...
There was a problem hiding this comment.
That line was commented-out since the first commit in vstinner@c5cf797#diff-39dd7d4b956a14a62f631dfb4c5f3fd4R14 but it's ultimately your call :) plistlib_generate_testdata.py was only modified by Ronald and @serhiy-storchaka: https://github.com/vstinner/cpython/commits/e0d2720ef5eccd9edb1ad82fc812b3a88e41a3fa/Mac/Tools/plistlib_generate_testdata.py And there are only 4 commits prior to your change. I'd say it's safe to remove it in this PR.
If you decide to keep it, then I'd suggest deleting NSPropertyListOpenStepFormat the import instead of commenting out it.
There was a problem hiding this comment.
Left this on to Ronald.
There was a problem hiding this comment.
Deleting the line NSPropertyListOpenStepFormat is fine.
There was a problem hiding this comment.
Deleting the line NSPropertyListOpenStepFormat is fine.
Ok, done.
| from email.policy import default | ||
|
|
||
| # If the e-mail headers are in a file, uncomment these two lines: | ||
| # If the e-mail headers are in a file, uncomment these three lines: |
There was a problem hiding this comment.
Let email experts to review this change.
| @@ -8,7 +8,7 @@ | |||
| from distutils.spawn import spawn | |||
| from distutils.file_util import move_file | |||
| from distutils.dir_util import mkpath | |||
| from distutils.dep_util import newer_pairwise, newer_group | |||
There was a problem hiding this comment.
Is this function used at all? In any case left this on distutils experts. This is very old and stable code. Maybe it is a bug that newer_pairwise is not used.
There was a problem hiding this comment.
It's part of the public API:
https://docs.python.org/dev/distutils/apiref.html#distutils.dep_util.newer_pairwise
This is very old and stable code. Maybe it is a bug that newer_pairwise is not used.
An unused import is a bug, I don't think that not using a function is a bug :-) The code uses newer_group().
There was a problem hiding this comment.
Yes, but maybe newer_pairwise() should be used instead of newer_group(). Or there is a typo in the name of newer_pairwise, and this is why it can't be searched in the code. Let distutils experts to review this change.
There was a problem hiding this comment.
Yes, but maybe newer_pairwise() should be used instead of newer_group(). Or there is a typo in the name of newer_pairwise, and this is why it can't be searched in the code. Let distutils experts to review this change.
I looked at the code, and I don't see how it could be a "typo". newer_pairwise() expects a list of objects in the second argument, whereas newer_group() expects a single "target" as the second argument. The code looks perfectly fine:
def _need_link(self, objects, output_file):
"""Return true if we need to relink the files listed in 'objects'
to recreate 'output_file'.
"""
if self.force:
return True
else:
if self.dry_run:
newer = newer_group (objects, output_file, missing='newer')
else:
newer = newer_group (objects, output_file)
return newer
There was a problem hiding this comment.
Seems good.
(In any case, if it was a mistake to use newer_group, the mistake has been here for a long time.
Changing it would need its own ticket.)
| @@ -1,10 +1,9 @@ | |||
| #!/usr/bin/env python3 | |||
|
|
|||
| from Cocoa import NSMutableDictionary, NSMutableArray, NSString, NSDate, NSNumber | |||
| from Cocoa import NSPropertyListSerialization, NSPropertyListOpenStepFormat | |||
| from Cocoa import NSPropertyListSerialization | |||
| # from Cocoa import NSPropertyListOpenStepFormat | |||
There was a problem hiding this comment.
Left this on to Ronald.
Will do on separate PR and backport.
Will do on separate PR and backport.
|
I will make idlelib changes on separate PRs attached to issues where they should have been made in the first place, and then backport to 3.6. I will also delete the now unused tabbedpages.py. I will then report on bpo-32156 For things like this, I would rather be made nosy on the bpo issue, with a paragraph added about the needed idlelib changes. |
|
Terry: "I will make idlelib changes on separate PRs attached to issues where they should have been made in the first place, and then backport to 3.6. I will also delete the now unused tabbedpages.py. I will then report on bpo-32156" Perfect! Terry: "For things like this, I would rather be made nosy on the bpo issue, with a paragraph added about the needed idlelib changes." I wasn't sure about the right granularity of such change which changes many files all round CPython code base. |
|
A separate PR for the idlelib changes would have been fine too, but it is slightly more work, which I am willing to do. |
Fix flake8 warning F401, ex: 'newer_pairwise' imported but unused.
https://bugs.python.org/issue32156