GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-4833: Add ZipFile.mkdir by dignissimus · Pull Request #32160 · python/cpython

Conversation

@dignissimus

@dignissimus dignissimus commented Mar 28, 2022 •
edited by merwok
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

https://bugs.python.org/issue4833

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
dignissimus added 2 commits March 28, 2022 20:14
@dignissimus
Add ZipFile.mkdir
6341b83
@dignissimus
Add news entry
5c441ad
@bedevere-bot bedevere-bot added the awaiting review label Mar 28, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 28, 2022
dignissimus added 2 commits March 28, 2022 20:33
@dignissimus
Remove example zip file
cc3c76e
@dignissimus
Allow directory ordering to vary in tests
1028df2
ethanfurman
ethanfurman reviewed Mar 28, 2022
View reviewed changes
Comment thread Lib/zipfile.py
else:
zinfo._compresslevel = self.compresslevel

if zinfo.is_dir():

@ethanfurman ethanfurman Mar 28, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ethanfurman ethanfurman Mar 30, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@merwok merwok Mar 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ethanfurman ethanfurman Mar 30, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ethanfurman
ethanfurman reviewed Mar 28, 2022
View reviewed changes

@ethanfurman ethanfurman left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Very thorough PR. Question in-line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dholth

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
merwok
merwok reviewed Mar 29, 2022
View reviewed changes
Comment thread Doc/library/zipfile.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@dignissimus
Move version directive to end of block
702f0d8
@ethanfurman

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ethanfurman

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dignissimus
Add fille type flag to ZipFile.mkdir, add ZipFIle.write directory tests
4a93d15
@dignissimus

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dignissimus
Remove redundant file type mask application
0b4877a
@merwok merwok changed the title bpo-4833: Adds ZipFile.mkdir bpo-4833: Add ZipFile.mkdir Apr 1, 2022
@dignissimus
Amend new entry
0387bfb
@dignissimus

dignissimus commented Apr 5, 2022

Copy link
Copy Markdown
Contributor Author

@ethanfurman
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ethanfurman
ethanfurman approved these changes Apr 5, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Apr 5, 2022
@ethanfurman
ethanfurman merged commit 050a8f9 into python:main Apr 5, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 5, 2022
@JelleZijlstra JelleZijlstra mentioned this pull request May 2, 2022
Add ZipFile.mkdir python/typeshed#7769
Merged
@schuppenies schuppenies mannequin mentioned this pull request Apr 10, 2022
Explicit directories for zipfiles #49083
Closed
@CAM-Gerlach CAM-Gerlach mentioned this pull request Oct 16, 2022
gh-95913: Edit zipfile Whatsnew section & add new APIs #98314
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@ethanfurman ethanfurman ethanfurman approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@dignissimus @dholth @ethanfurman @merwok @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.