Skip to content

bpo-39667: Sync zipp 3.0 #18540

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

Merged
jaraco merged 5 commits into from
Feb 29, 2020
Merged

bpo-39667: Sync zipp 3.0 #18540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b178695
bpo-39667: Improve pathlib.Path compatibility on zipfile.Path and cor…
jaraco Feb 17, 2020
7a827ff
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 17, 2020
671ab3e
Update docs for new zipfile.Path.open
jaraco Feb 22, 2020
3d9272a
Rely on dict, faster than OrderedDict.
jaraco Feb 22, 2020
34d7114
Syntax edits on docs
jaraco Feb 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions Doc/library/zipfile.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,10 +489,20 @@ Path objects are traversable using the ``/`` operator.

The final path component.

.. method:: Path.open(*, **)

Invoke :meth:`ZipFile.open` on the current path. Accepts
the same arguments as :meth:`ZipFile.open`.
.. method:: Path.open(mode='r', *, pwd, **)

Invoke :meth:`ZipFile.open` on the current path.
Allows opening for read or write, text or binary
through supported modes: 'r', 'w', 'rb', 'wb'.
Positional and keyword arguments are passed through to
:class:`io.TextIOWrapper` when opened as text and
ignored otherwise.
``pwd`` is the ``pwd`` parameter to
:meth:`ZipFile.open`.

.. versionchanged:: 3.9
Added support for text and binary modes for open. Default
mode is now text.

.. method:: Path.iterdir()

Expand Down
8 changes: 7 additions & 1 deletion Lib/test/test_zipfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import os
import pathlib
import posixpath
import string
import struct
import subprocess
import sys
Expand Down Expand Up @@ -2880,7 +2881,7 @@ def test_open(self):
a, b, g = root.iterdir()
with a.open() as strm:
data = strm.read()
assert data == b"content of a"
assert data == "content of a"

@tirkarthi tirkarthi Feb 19, 2020

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.

This changes the return type of Zipfile.Path.open from bytes to string and could be backwards incompatible on backporting to 3.8

@jaraco jaraco Feb 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for highlighting this concern. It's this concern that's kept me from fast-tracking this change. I'll try to enumerate the rationale and concerns and alternatives (to simply breaking it).

Rationale

  • zipfile.Path was intended to be compatible with a subset of pathlib.Path.
  • Recent efforts in importlib_resources revealed that a pathlib.Path compatible implementation of .open() would be useful if not necessary.
  • It would be beneficial for users for the two Path objects to have a consistent and compatible interface.
  • The .open() probably should have been kept as a private (._open()) interface until it was needed.
  • Users relying on .open() today can readily call Path.root.open() to get the current behavior.
  • Impacted users of backport (zipp) would have already been exposed to this change (via zipp 3.0) and likely have adapted. No concerns were raised.

Concerns

  • Users of Python 3.8 may be relying on the existing .open() implementation.
  • The documentation explicitly declares support for the existing behavior of .open(). If not for this concern, one might have been able to make the argument that .open() was an internal implementation detail.
  • The "compatible subset" of pathlib.Path has previously been only informally defined, although there is an ABC in importlib_resources (slated for inclusion in Python 3.9) that aims to formalize that interface.
  • There's no usage a user could supply that would work on both .open() methods as currently implemented. pathlib.Path requires mode='rb' to return bytes and zipfile.Path fails if 'b' is passed.
  • The interface isn't just used by callers of zipfile, but also by consumers of APIs that return zipfile.Path (i.e. importlib.metadata).

Alternatives

I'm considering several possible approaches.

  • Simply break compatibility. The impact may be negligible given the limited exposure of zipfile.Path.
  • Exclude this change in behavior from the (3.8) backport... and simply require all Python 3.8 users to deal with the incompatible interface.
  • Provide a compatibility method like _future_open only on Python 3.8 that users could call to get the future behavior.
  • Provide a compatible subclass, FuturePath only on Python 3.8 that would implement the alternate interface and would be supplied by importlib.metadata and others.
  • Provide a backward-compatibility method like _legacy_open that provides the legacy behavior. That seems unnecessary given that zipfile.Path.root.open exists.

Regardless of how we proceed, there are some actions.

  • Update docs to reflect change in API.

@jaraco jaraco Feb 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After mulling this over throughout the week and capturing the rationale and concerns above, I'm leaning toward making this backward-incompatible change in Python 3.8.x, assuming you agree and think the concern is manageable. Here's how that would work:

  • Backport this code change as presented in this PR.
  • Docs would be updated to reflect the change happening in 3.8.(next).
  • "What's New" would describe for users relying on zipfile.Path.open in Python 3.8.(next-1) to use zipfile.Path.root.open instead for easy, straightforward compatibility with the old behavior.

@ambv What do you think? Let me know if there's a better forum in which to discuss.


def test_read(self):
for alpharep in self.zipfile_alpharep():
Expand Down Expand Up @@ -2974,6 +2975,11 @@ def test_joinpath_constant_time(self):
# Check the file iterated all items
assert entries.count == self.HUGE_ZIPFILE_NUM_ENTRIES

# @func_timeout.func_set_timeout(3)
def test_implied_dirs_performance(self):
data = ['/'.join(string.ascii_lowercase + str(n)) for n in range(10000)]
zipfile.CompleteDirs._implied_dirs(data)


if __name__ == "__main__":
unittest.main()
63 changes: 31 additions & 32 deletions Lib/zipfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import threading
import time
import contextlib
from collections import OrderedDict

try:
import zlib # We may need its compression method
Expand Down Expand Up @@ -2102,24 +2101,6 @@ def _compile(file, optimize=-1):
return (fname, archivename)


def _unique_everseen(iterable, key=None):
"List unique elements, preserving order. Remember all elements ever seen."
# unique_everseen('AAAABBBCCDAABBB') --> A B C D
# unique_everseen('ABBCcAD', str.lower) --> A B C D
seen = set()
seen_add = seen.add
if key is None:
for element in itertools.filterfalse(seen.__contains__, iterable):
seen_add(element)
yield element
else:
for element in iterable:
k = key(element)
if k not in seen:
seen_add(k)
yield element


def _parents(path):
"""
Given a path with elements separated by
Expand Down Expand Up @@ -2161,6 +2142,18 @@ def _ancestry(path):
path, tail = posixpath.split(path)


_dedupe = dict.fromkeys
"""Deduplicate an iterable in original order"""


def _difference(minuend, subtrahend):
"""
Return items in minuend not in subtrahend, retaining order
with O(1) lookup.
"""
return itertools.filterfalse(set(subtrahend).__contains__, minuend)


class CompleteDirs(ZipFile):
"""
A ZipFile subclass that ensures that implied directories
Expand All @@ -2170,13 +2163,8 @@ class CompleteDirs(ZipFile):
@staticmethod
def _implied_dirs(names):
parents = itertools.chain.from_iterable(map(_parents, names))
# Deduplicate entries in original order
implied_dirs = OrderedDict.fromkeys(
p + posixpath.sep for p in parents
# Cast names to a set for O(1) lookups
if p + posixpath.sep not in set(names)
)
return implied_dirs
as_dirs = (p + posixpath.sep for p in parents)
return _dedupe(_difference(as_dirs, names))

def namelist(self):
names = super(CompleteDirs, self).namelist()
Expand Down Expand Up @@ -2305,20 +2293,31 @@ def __init__(self, root, at=""):
self.root = FastLookup.make(root)
self.at = at

@property
def open(self):
return functools.partial(self.root.open, self.at)
def open(self, mode='r', *args, **kwargs):
"""
Open this entry as text or binary following the semantics
of ``pathlib.Path.open()`` by passing arguments through
to io.TextIOWrapper().
"""
pwd = kwargs.pop('pwd', None)
zip_mode = mode[0]
stream = self.root.open(self.at, zip_mode, pwd=pwd)
if 'b' in mode:
if args or kwargs:
raise ValueError("encoding args invalid for binary operation")
return stream
return io.TextIOWrapper(stream, *args, **kwargs)

@property
def name(self):
return posixpath.basename(self.at.rstrip("/"))

def read_text(self, *args, **kwargs):
with self.open() as strm:
return io.TextIOWrapper(strm, *args, **kwargs).read()
with self.open('r', *args, **kwargs) as strm:
return strm.read()

def read_bytes(self):
with self.open() as strm:
with self.open('rb') as strm:
return strm.read()

def _is_child(self, path):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2020-02-17-22-38-15.bpo-39667.QuzEHH.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve pathlib.Path compatibility on zipfile.Path and correct performance degradation as found in zipp 3.0.