Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-94844: Add pathlib support to shutil archive management #94846

Merged
merged 12 commits into from
Jul 20, 2022
Merged

gh-94844: Add pathlib support to shutil archive management #94846

merged 12 commits into from
Jul 20, 2022

Conversation

arhadthedev
Copy link
Member

@arhadthedev arhadthedev commented Jul 14, 2022 •
edited
Loading

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

@arhadthedev arhadthedev changed the title gh-94844: Fix shutil makearchive gh-94844: Add pathlib support to shutil archive management Jul 14, 2022
@barneygale
Copy link
Contributor

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

@arhadthedev arhadthedev marked this pull request as draft July 14, 2022 08:33
@arhadthedev arhadthedev marked this pull request as ready for review July 14, 2022 08:44
@arhadthedev
Copy link
Member Author

@barneygale Thank you, addressed.

@barneygale
Copy link
Contributor

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

@arhadthedev
Copy link
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.

@barneygale
Copy link
Contributor

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

barneygale
barneygale reviewed Jul 14, 2022
View reviewed changes
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
Copy link
Contributor

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.

Copy link
Member Author

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.

serhiy-storchaka
serhiy-storchaka reviewed Jul 16, 2022
View reviewed changes
Lib/shutil.py Outdated
@@ -1127,6 +1127,7 @@ def make_archive(base_name, format, root_dir=None, base_dir=None, verbose=0,
if not dry_run:
os.chdir(root_dir)

base_name = os.fspath(base_name)
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, amended.

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
Copy link
Member

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.

Copy link
Member Author

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?

barneygale
barneygale reviewed Jul 16, 2022
View reviewed changes
@serhiy-storchaka serhiy-storchaka merged commit ed44415 into python:main Jul 20, 2022
@serhiy-storchaka serhiy-storchaka added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jul 20, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes 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
Copy link

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

@arhadthedev
Copy link
Member Author

@serhiy-storchaka Thanks for merging.

@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
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

@bateller bateller bateller approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@arhadthedev @barneygale @miss-islington @bedevere-bot @serhiy-storchaka @bateller @AlexWaygood