FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-4833: Add ZipFile.mkdir by dignissimus · Pull Request #32160 · python/cpython · GitHub

/ cpython Public

bpo-4833: Add ZipFile.mkdir - #32160

Merged
ethanfurman merged 8 commits into
python:mainfrom
dignissimus:bpo-1590744
Apr 5, 2022
ConversationCommits (8)ChecksFiles changed
Merged

bpo-4833: Add ZipFile.mkdir#32160
ethanfurman merged 8 commits into
python:mainfrom
dignissimus:bpo-1590744

Conversation

dignissimus commented Mar 28, 2022 •
edited by merwok
Loading

Copy link
Copy Markdown
Contributor

Add ZipFile.mkdir method to create directories inside zip archives.

https://bugs.python.org/issue4833

ethanfurman reviewed Mar 28, 2022
View reviewed changes
Comment thread Lib/zipfile.py
else:
zinfo._compresslevel = self.compresslevel

if zinfo.is_dir():

ethanfurman Mar 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does this mean that write can no longer be used to create a directory? I suspect that will break existing code.

ethanfurman Mar 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This was marked as resolved, but I do not see an answer to my question.

merwok Mar 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think write still accepts directory zinfo, now it’s handled on line 1775 by calling mkdir instead of directly inline here.
There should be a test for write + directory; if not, then this PR needs to add one!

ethanfurman Mar 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, right -- 1175 does handle that. Thanks!

A fairly quick scan of the tests doesn't reveal one using .write(some_directory/). @dignissimus please confirm such a test already exists, or add one.

ethanfurman reviewed Mar 28, 2022
View reviewed changes

ethanfurman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Very thorough PR. Question in-line.

dholth commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

interesting, since it is also possible to imply directories (if there is a file in there) but this would allow an empty directory

merwok reviewed Mar 29, 2022
View reviewed changes
Comment thread Doc/library/zipfile.rst Outdated

ethanfurman commented Mar 31, 2022

Copy link
Copy Markdown
Member

@dignissimus : ping me in a few days if no one has merged this yet -- I want to give other folks the chance to review.

ethanfurman commented Mar 31, 2022

Copy link
Copy Markdown
Member

@dignissimus Actually, I forgot: have you either verified or added a test for creating a directory using the existing write() method? (Yes, it should work -- nevertheless, a test is good).

dignissimus commented Apr 1, 2022

Copy link
Copy Markdown
Contributor Author

@dignissimus Actually, I forgot: have you either verified or added a test for creating a directory using the existing write() method? (Yes, it should work -- nevertheless, a test is good).

@ethanfurman The tests have been added!

merwok changed the title bpo-4833: Adds ZipFile.mkdir bpo-4833: Add ZipFile.mkdir Apr 1, 2022

dignissimus commented Apr 5, 2022

Copy link
Copy Markdown
Contributor Author

@ethanfurman

ethanfurman merged commit 050a8f9 into python:main Apr 5, 2022
JelleZijlstra mentioned this pull request May 2, 2022
Merged
schuppenies mannequin mentioned this pull request Apr 10, 2022
Closed
CAM-Gerlach mentioned this pull request Oct 16, 2022
Merged
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

merwok merwok left review comments
ethanfurman ethanfurman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL