Skip to content

Fix incorrect keyword-only arguments in tarfile.open() #13814

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.

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
merged 2 commits into from
Apr 11, 2025

Conversation

edwardpeek-crown-public
Copy link
Contributor

@edwardpeek-crown-public edwardpeek-crown-public commented Apr 11, 2025

Per https://docs.python.org/3/library/tarfile.html#tarfile.open all of name, mode, fileobj, and bufsize are permitted to be positional.

@edwardpeek-crown-public edwardpeek-crown-public marked this pull request as draft April 11, 2025 04:17

This comment has been minimized.

This comment has been minimized.

@edwardpeek-crown-public edwardpeek-crown-public force-pushed the patch-1 branch 2 times, most recently from 723526b to 8efb781 Compare April 11, 2025 04:56

This comment has been minimized.

@edwardpeek-crown-public edwardpeek-crown-public marked this pull request as ready for review April 11, 2025 05:09
@edwardpeek-crown-public
Copy link
Contributor Author

Took me a while to wrap my head around the signature duplication, but I think this fixes the overly constrained pipe modes (from #13130) in the right spirit.

@hauntsaninja
Copy link
Collaborator

Could you add a test case corresponding to the thing you'd like to pass type check to stdlib/@tests/test_cases/check_tarfile.py?

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

vision (https://github.com/pytorch/vision)
+ torchvision/datasets/utils.py:215: note:     def open(cls, name: str | bytes | PathLike[str] | PathLike[bytes] | Buffer | None, mode: Literal['r|*', 'r|', 'r|gz', 'r|bz2', 'r|xz'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ torchvision/datasets/utils.py:215: note:     def open(cls, name: str | bytes | PathLike[str] | PathLike[bytes] | Buffer | None, mode: Literal['w|', 'w|xz'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ...) -> TarFile
+ torchvision/datasets/utils.py:215: note:     def open(cls, name: str | bytes | PathLike[str] | PathLike[bytes] | Buffer | None, mode: Literal['w|gz', 'w|bz2'], fileobj: _Fileobj | None = ..., bufsize: int = ..., *, format: int | None = ..., tarinfo: type[TarInfo] | None = ..., dereference: bool | None = ..., ignore_zeros: bool | None = ..., encoding: str | None = ..., errors: str = ..., pax_headers: Mapping[str, str] | None = ..., debug: int | None = ..., errorlevel: int | None = ..., compresslevel: int = ...) -> TarFile

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks!

Unfortunately, the fact that arguments with defaults can't precede arguments without defaults makes these extra overloads necessary. This was previously discussed on the typing-sig mailing list and subsequently on the python-dev mailing list. Maybe it's time to resurrect that proposal, possibly with the support of the typing council.

@srittau srittau merged commit bb1cbfa into python:main Apr 11, 2025
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants