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

Accurate overloads for ZipFile.__init__ #12119

Merged
merged 19 commits into from
Aug 11, 2024
Merged

Accurate overloads for ZipFile.__init__ #12119

Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c2505eb
chore: Accurate overloads for `ZipFile.__init__`
max-muoto Jun 8, 2024
6eafa4e
Tweak
max-muoto Jun 8, 2024
6bc28bd
Tweak
max-muoto Jun 8, 2024
4cf07ab
Add test
max-muoto Jun 9, 2024
7f75295
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 9, 2024
c9704b9
Tweaks
max-muoto Jun 9, 2024
377ce76
Tweak
max-muoto Jun 9, 2024
aca61a7
Merge branch 'main' into accurate-zip-overloads
max-muoto Jun 9, 2024
3af922f
Add missing annotations
max-muoto Jun 9, 2024
0f7c9e0
Missing annotation
max-muoto Jun 9, 2024
d82e36f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 9, 2024
97a4a95
Use positional arguments
max-muoto Jun 22, 2024
a682d36
Merge branch 'main' into accurate-zip-overloads
max-muoto Jun 23, 2024
0b52c6a
Address feedback
max-muoto Jul 5, 2024
814ab6b
Shorten name
max-muoto Jul 5, 2024
524d7d7
Fix tests
max-muoto Jul 5, 2024
56d44c0
Merge branch 'main' into accurate-zip-overloads
max-muoto Aug 3, 2024
d98196a
Address feedback
max-muoto Aug 11, 2024
cc520e2
Fix
max-muoto Aug 11, 2024
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
78 changes: 76 additions & 2 deletions stdlib/zipfile/__init__.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,20 @@ class ZipExtFile(io.BufferedIOBase):
class _Writer(Protocol):
def write(self, s: str, /) -> object: ...

class _ZipReadable(Protocol):
def seek(self, offset: int, whence: int = 0) -> int: ...
max-muoto marked this conversation as resolved.
Show resolved Hide resolved
def read(self, n: int | None = -1) -> bytes: ...

class _ZipTellable(Protocol):
def tell(self) -> int: ...

class _ZipSeekTellable(_ZipTellable):
def seek(self, offset: int, whence: int = 0) -> int: ...

class _ZipWritable(_Writer):
def flush(self) -> None: ...
def close(self) -> None: ...

class ZipFile:
filename: str | None
debug: int
Expand All @@ -106,11 +120,25 @@ class ZipFile:
compresslevel: int | None # undocumented
mode: _ZipFileMode # undocumented
pwd: bytes | None # undocumented
# metadata_encoding is new in 3.11
if sys.version_info >= (3, 11):
@overload
def __init__(
self,
file: StrPath | IO[bytes],
mode: _ZipFileMode = "r",
compression: int = 0,
allowZip64: bool = True,
compresslevel: int | None = None,
*,
strict_timestamps: bool = True,
metadata_encoding: str | None = None,
) -> None: ...
Comment on lines 126 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't all following overloads shadowed by this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - for example, IO[bytes] is broader than _ZipReadable, which will lead to the second overload getting chosen if you just an object that is readable.

Copy link
Contributor Author

@max-muoto max-muoto Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As for _ZipFileMode, if file doesn't match we'll end up skipping over the overload. The tests show that this is working properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IO[bytes] is broader than _ZipReadable

But that's exactly the problem. It's my understanding that overloads are processed in order (although the typing spec doesn't mention that), so IO[bytes] always matches before _ZipReadable can match.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've opened python/typing#1803 for clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is that a problem though? _ZipReadable will get matched if IO[bytes] isn't fully fulfilled, or am I misunderstanding something?

# metadata_encoding is only allowed for read mode
@overload
def __init__(
self,
file: StrPath | _ZipReadable,
mode: Literal["r"] = "r",
compression: int = 0,
allowZip64: bool = True,
Expand All @@ -122,8 +150,20 @@ class ZipFile:
@overload
def __init__(
self,
file: StrPath | IO[bytes],
mode: _ZipFileMode = "r",
file: StrPath | _ZipTellable | _ZipWritable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

While _ZipTellable won't immediately error, I don't think the resulting object can be used for anything right? Unless I'm missing some use case, I think we should keep it StrPath | _ZipWritable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh in fact it's definitely wrong, because __del__ will trigger close will trigger a write attempt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d98196a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hauntsaninja Should be resolved now.

mode: Literal["w", "x"] = ...,
compression: int = 0,
allowZip64: bool = True,
compresslevel: int | None = None,
*,
strict_timestamps: bool = True,
metadata_encoding: None = None,
) -> None: ...
@overload
def __init__(
self,
file: StrPath | _ZipReadable | _ZipSeekTellable,
mode: Literal["a"] = ...,
compression: int = 0,
allowZip64: bool = True,
compresslevel: int | None = None,
Expand All @@ -132,6 +172,7 @@ class ZipFile:
metadata_encoding: None = None,
) -> None: ...
else:
@overload
def __init__(
self,
file: StrPath | IO[bytes],
Expand All @@ -142,6 +183,39 @@ class ZipFile:
*,
strict_timestamps: bool = True,
) -> None: ...
@overload
def __init__(
self,
file: StrPath | _ZipReadable,
mode: Literal["r"] = "r",
compression: int = 0,
allowZip64: bool = True,
compresslevel: int | None = None,
*,
strict_timestamps: bool = True,
) -> None: ...
@overload
def __init__(
self,
file: StrPath | _ZipTellable | _ZipWritable,
mode: Literal["w", "x"] = ...,
compression: int = 0,
allowZip64: bool = True,
compresslevel: int | None = None,
*,
strict_timestamps: bool = True,
) -> None: ...
@overload
def __init__(
self,
file: StrPath | _ZipReadable | _ZipSeekTellable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this right? I'm not sure _ZipSeekTellable is enough, e.g. _RealGetContents requires read

Copy link
Contributor Author

@max-muoto max-muoto Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, we want a protocol that is seekable/readable if it's a valid zip file, otherwise it should implement tell: 0b52c6a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think likely misread this, and assumed if an attribute error occurred it just needed to be tellable.

mode: Literal["a"] = ...,
compression: int = 0,
allowZip64: bool = True,
compresslevel: int | None = None,
*,
strict_timestamps: bool = True,
) -> None: ...

def __enter__(self) -> Self: ...
def __exit__(
Expand Down