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 — gh-94844: Add pathlib support to shutil archive management by arhadthedev · Pull Request #94846 · python/cpython

Conversation

@arhadthedev

@arhadthedev arhadthedev commented Jul 14, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Underlying ZipFile.__init__ casts a path to str anyway:

cpython/Lib/zipfile.py

Lines 1270 to 1271 in b03a9e8

if isinstance(file, os.PathLike):
file = os.fspath(file)

so make_archive can safely cast its path param to a str as well (including implicit conversion in f-strings).

  • Issue: shutil.make_archive now fails with pathlib.Path object in Python 3.11.0b4 #94844

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@arhadthedev
Add pathlib support to shutil.make_archive
3912f2e
@bedevere-bot bedevere-bot added the awaiting review label Jul 14, 2022
@arhadthedev
Add pathlib support to shutil._make_tarball
828abde
@arhadthedev arhadthedev changed the title gh-94844: Fix shutil makearchive gh-94844: Add pathlib support to shutil archive management Jul 14, 2022
@barneygale

barneygale commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

make_archive() could be adjusted to call base_name = os.fspath(base_name).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@arhadthedev
Extend TestArchives with pathlib cases
49a2158
@arhadthedev
arhadthedev marked this pull request as draft July 14, 2022 08:33
arhadthedev added 2 commits July 14, 2022 11:39
@arhadthedev
Address a Barney's review
deebebc
@arhadthedev
Fix accidental passing of self into a non-method
0e14249
@arhadthedev
arhadthedev marked this pull request as ready for review July 14, 2022 08:44
@arhadthedev

arhadthedev commented Jul 14, 2022

Copy link
Copy Markdown
Member Author

@barneygale Thank you, addressed.
All reactions

Sorry, something went wrong.

Uh oh!

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

barneygale commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

Could you call os.fspath() in make_archive() rather than _make_zipfile() and _make_tarball()?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@arhadthedev
Move os.fspath call into a higher level
26d736c
@arhadthedev

arhadthedev commented Jul 14, 2022

Copy link
Copy Markdown
Member Author

Fixed; I quiet misunderstood first.

Currently I put the conversion after sys.audit to not break backward compatibility and provide audit listeners raw input data. I can change this if needed.
All reactions

Sorry, something went wrong.

Uh oh!

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

barneygale commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

Could you remove the changes to _make_zipfile() and _make_tarball() please?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
barneygale
barneygale reviewed Jul 14, 2022
View reviewed changes
Comment thread Lib/test/test_shutil.py Outdated
Comment on lines +1707 to +1712
class TestPathlibPathArchives(_ArchiveTests, unittest.TestCase):

def _make_archive_with_path(self, path, /, *args, **kwargs):
return make_archive(pathlib.Path(path), *args, **kwargs)

_make_archive = _make_archive_with_path

@barneygale barneygale Jul 14, 2022

Copy link
Copy Markdown
Contributor

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.

No need to add a class. A single test will suffice.

Sorry, something went wrong.

Uh oh!

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

@arhadthedev arhadthedev Jul 14, 2022

Copy link
Copy Markdown
Member Author

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.

Reverted; I agree that after the conversion moved into make_archive frontend, there is no need to explicitly test _make_* backends.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
arhadthedev added 2 commits July 14, 2022 14:34
@arhadthedev
Revert the extra set of tests and the replacement with f-strings
85bf9f1
@arhadthedev
Restore a newline missing in Github raw view
f099b32
bateller
bateller approved these changes Jul 14, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jul 14, 2022
serhiy-storchaka
serhiy-storchaka reviewed Jul 16, 2022
View reviewed changes
Comment thread Lib/shutil.py Outdated
if not dry_run:
os.chdir(root_dir)

base_name = os.fspath(base_name)

@serhiy-storchaka serhiy-storchaka Jul 16, 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.

Please do it only if root_dir is not None, and only if support_root_dir is true (because os.path.abspath() already does the work).

It is an undocumented behavior and we do not want to introduce a new official feature in a bugfix release.

Sorry, something went wrong.

Uh oh!

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

@arhadthedev arhadthedev Jul 16, 2022

Copy link
Copy Markdown
Member Author

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.

Thank you, amended.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2022-07-14-10-28-10.gh-issue-94844.NJ3W80.rst Outdated
@@ -0,0 +1,2 @@
:meth:`shutil.make_archive` now accepts any :class:`os.PathLike`. Patch by

@serhiy-storchaka serhiy-storchaka Jul 16, 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.

It was undocumented behavior, and we do not yet guarantee it in future versions, so no NEWS entry is needed.

Sorry, something went wrong.

Uh oh!

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

@arhadthedev arhadthedev Jul 16, 2022

Copy link
Copy Markdown
Member Author

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.

Done; could you add skip-news label please?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
arhadthedev added 2 commits July 16, 2022 16:49
@arhadthedev
Merge branch 'main' into fix-shutil-makearchive
debd99b
@arhadthedev
Address the Serhiy's review
ff94966
@AlexWaygood AlexWaygood added the skip news label Jul 16, 2022
barneygale
barneygale reviewed Jul 16, 2022
View reviewed changes
Comment thread Lib/shutil.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
arhadthedev added 2 commits July 16, 2022 18:56
@arhadthedev
Address the Barney's review
bdaf3f9
@arhadthedev
Add a forgotten comment from the review
5a1c347
serhiy-storchaka
serhiy-storchaka approved these changes Jul 20, 2022
View reviewed changes
@bedevere-bot bedevere-bot removed the awaiting core review label Jul 20, 2022
@bedevere-bot bedevere-bot added the awaiting merge label Jul 20, 2022
@serhiy-storchaka
serhiy-storchaka merged commit ed44415 into python:main Jul 20, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 20, 2022
@serhiy-storchaka serhiy-storchaka added needs backport to 3.10 needs backport to 3.11 only security fixes labels Jul 20, 2022
@miss-islington

miss-islington commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Thanks @arhadthedev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington

miss-islington commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Thanks @arhadthedev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Jul 20, 2022

Copy link
Copy Markdown

GH-95054 is a backport of this pull request to the 3.10 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Jul 20, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2022
@arhadthedev @miss-islington
pythongh-94844: Add pathlib support to shutil archive management (pyt…
cadf202 
…honGH-94846)

Co-authored-by: Barney Gale <[email protected]>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2022
@arhadthedev @miss-islington
pythongh-94844: Add pathlib support to shutil archive management (pyt…
c7d812d 
…honGH-94846)

Co-authored-by: Barney Gale <[email protected]>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 20, 2022
@bedevere-bot

bedevere-bot commented Jul 20, 2022

Copy link
Copy Markdown

GH-95055 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

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

arhadthedev commented Jul 20, 2022

Copy link
Copy Markdown
Member Author

@serhiy-storchaka Thanks for merging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@arhadthedev
arhadthedev deleted the fix-shutil-makearchive branch July 20, 2022 16:01
miss-islington added a commit that referenced this pull request Jul 20, 2022
@miss-islington @arhadthedev
gh-94844: Add pathlib support to shutil archive management (GH-94846)
6cc9489 
Co-authored-by: Barney Gale <[email protected]>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <[email protected]>
miss-islington added a commit that referenced this pull request Jul 20, 2022
@miss-islington @arhadthedev
gh-94844: Add pathlib support to shutil archive management (GH-94846)
cbe3ad5 
Co-authored-by: Barney Gale <[email protected]>
(cherry picked from commit ed44415)

Co-authored-by: Oleg Iarygin <[email protected]>
@gvanrossum gvanrossum mentioned this pull request Oct 4, 2022
shutil.make_archive now fails with pathlib.Path object in Python 3.11.0b4 #94844
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@barneygale barneygale barneygale left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
@bateller bateller bateller approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

7 participants

@arhadthedev @barneygale @miss-islington @bedevere-bot @serhiy-storchaka @bateller @AlexWaygood
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.