NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

bpo-32742: use infolist for zipfile.ZipFile.extractall#5472

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

peterbe wants to merge3 commits intopython:main

base:main

Choose a base branch

frompeterbe:use-infolist-for-zipefileextractall

Open

bpo-32742: use infolist for zipfile.ZipFile.extractall#5472

peterbe wants to merge3 commits intopython:mainfrompeterbe:use-infolist-for-zipefileextractall

+6 −4

Conversation

Copy link

peterbe commentedFeb 1, 2018•
edited by bedevere-bot
Loading

https://bugs.python.org/issue32742

Copy link

the-knights-who-say-ni commentedFeb 1, 2018

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please followthe steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

the-knights-who-say-ni added the CLA not signed label

Feb 1, 2018

bedevere-bot added the awaiting review label

Feb 1, 2018

bpo-32742: use infolist for zipfile.ZipFile.extractall

1bb1df5

zipfile.ZipFile class's extractall method allows the list of members to beunset and if it is, defaults to the list of file *names* in the zip. Whensent to _extract_member that file name needs to be rewrapped as a ZipInfoinstance. The ZipInfo instances is already available inside extractall andcan be re-used.

peterbe force-pushed theuse-infolist-for-zipefileextractall branch fromd802d06 to1bb1df5Compare

February 1, 2018 15:23

merwok reviewed

Feb 4, 2018

View reviewed changes

Lib/zipfile.pyShow resolvedHide resolved

update doc string

8d4c709

the-knights-who-say-ni added CLA signed and removed CLA not signed labels

Feb 4, 2018

merwok reviewed

Feb 5, 2018

View reviewed changes

Lib/zipfile.py Outdated

		@@ -1580,10 +1580,10 @@ def extractall(self, path=None, members=None, pwd=None):
		"""Extract all members from the archive to the current working
		directory. `path' specifies a different directory to extract to.
		`members' is optional and must be a subset of the list returned
		bynamelist().
		byinfolist().

Copy link

Member

merwokFeb 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think more doc is needed (here and in the ReST documentation) to make things clear. The code change in this PR does not change the function interface: people could pass a list of names or a list of ZipInfos before (thanks to extractall calling _extract_member which supports both), and they’re still able to do that after the change.

Maybe the doc could be explicit about these two options? Instead of naming directly namelist or infolist (and letting people think that members must be strings or must be ZipInfos), something like «members is optional and must contain strings or ZipInfo objects corresponding to valid entries (i.e. a subset of the list returned by namelist or infolist)».

Copy link

Author

peterbeFeb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks. I juggled the idea but everything I attempted just got long winded and more confusing. Just sayinginfolist ornamelist is both correct and really brief. Too brief?