Python 3.14: PEP-784 compression.zstd by Rogdham · Pull Request #14129 · python/typeshed

Rogdham · 2025-05-24T11:05:31Z

Second PR about PEP-784 in stdlib (first PR):

Add type hints for _zstd
Add type hints for compression.zstd

Rogdham · 2025-05-24T12:19:50Z

@emmatyping: if you want to give try this out, or simply comment on the use of Mapping[CompressionParameter, int] and Mapping[DecompressionParameter, int] as type hints for the options parameter. Any feedback would be appreciated!

emmatyping · 2025-05-24T15:04:53Z

Note that I typed the options as Mapping[CompressionParameter, int] / Mapping[DecompressionParameter, int], while a Mapping[int, int] is technically accepted.

I think we should type that it accepts Mapping[int, int] as well. I consider that a supported usage.

Rogdham · 2025-05-29T07:41:01Z

Now that CPython 3.14.0 beta 2 is released, the CI is green and the PR ready for review!

srittau

Thanks, a few remark below. Also I notice that many defaults are ..., although they have basic defaults in the implementation. In these cases, we include the actual defaults, ... is only used for complicated cases.

srittau · 2025-05-30T14:47:43Z

+    CONTINUE: Final[_ZstdCompressorContinue] = 0
+    FLUSH_BLOCK: Final[_ZstdCompressorFlushBlock] = 1
+    FLUSH_FRAME: Final[_ZstdCompressorFlushFrame] = 2


The Final implies the Literal:

Suggested change

CONTINUE: Final[_ZstdCompressorContinue] = 0

FLUSH_BLOCK: Final[_ZstdCompressorFlushBlock] = 1

FLUSH_FRAME: Final[_ZstdCompressorFlushFrame] = 2

CONTINUE: Final = 0

FLUSH_BLOCK: Final = 1

FLUSH_FRAME: Final = 2

That was to address a suggestion of @emmatyping of making sure the value is only defined once.

Do you want me to revert all occurrences of _ZstdCompressorContinue?

e.g. replace _ZstdCompressorContinue | _ZstdCompressorFlushBlock | _ZstdCompressorFlushFrame down below with Literal[0, 1, 2]

The rest is fine as is. This was just to point out the redundancy here.

srittau · 2025-05-30T14:49:07Z

+def get_frame_info(frame_buffer: ReadableBuffer) -> tuple[int, int]: ...
+def get_frame_size(frame_buffer: ReadableBuffer) -> int: ...
+def get_param_bounds(parameter: int, is_compress: bool) -> tuple[int, int]: ...
+def set_parameter_types(c_parameter_type: type[Any], d_parameter_type: type[Any]) -> None: ...


Why use Any here? I assume that only certain types are allowed? In that case, we need a comment explaining that.

Yeah these should be specifically type[CompressionParameter] and type[DecompressionParameter].

Suggested change

def set_parameter_types(c_parameter_type: type[Any], d_parameter_type: type[Any]) -> None: ...

def set_parameter_types(c_parameter_type: type[CompressionParameter], d_parameter_type: type[DecompressionParameter]) -> None: ...

If my understanding is correct, this functions take any types, and will set valid types to be used as keys (on top of int) for the options parameters.

In the implementation of zstd a single call is done explicitly with CompressionParameter and DecompressionParameter, but it could have been something else.

In any case, it is in a private _zstd module, so I don't think it matters a lot.

Happy to change this with Emma's suggestion.

While they can take any type, I would argue it is wrong to do so. I guess you could specify type[IntEnum] perhaps if you didn't want to specify the specific types?

I think you are right, and typing with type[CompressionParameter]/type[DecompressionParameter] better shows the intent.

I will go with that.

srittau · 2025-05-30T14:53:58Z

+_PathOrFileBinary: TypeAlias = StrOrBytesPath | IO[bytes]
+_PathOrFileText: TypeAlias = StrOrBytesPath | IO[str]


We try to avoid the unwieldy pseudo-protocol IO and its sub-classes, especially in argument annotations. In this case, appropriate protocols seem to be fairly to construct.

I took this from the lzma.pyi file so I thought it was ok. Tell me what you think.

The lzma stubs probably predate the introduction of protocols and need to be updated as well at some point.

srittau · 2025-05-30T14:56:30Z

+    encoding: str | None = ...,
+    errors: str | None = ...,
+    newline: str | None = ...,
+) -> TextIO: ...


Same here. The implementation seems to return TextIOWrapper, so we should just return this concrete type.

I took this from the lzma.pyi file so I thought it was ok.

This one is straightforward to change though. Do you want me to change it in lzma.pyi as well?

Changing it in lzma as well would be appreciated.

srittau · 2025-05-30T14:56:36Z

+    encoding: str | None = ...,
+    errors: str | None = ...,
+    newline: str | None = ...,
+) -> TextIO: ...


Rogdham · 2025-05-31T08:08:56Z

Also I notice that many defaults are ..., although they have basic defaults in the implementation.

I thought that was implementation details with no use for type checking, but I will change.

Likewise, is it needed to define the values for the constants?

For example:

# currently
ZSTD_CLEVEL_DEFAULT: Final[int]

# possible change
ZSTD_CLEVEL_DEFAULT: Final = 3

srittau · 2025-05-31T10:07:55Z

Likewise, is it needed to define the values for the constants?

Generally we do that nowadays.

Rogdham · 2025-05-31T16:23:40Z

I think I have addressed all your points, feel free to review again, especially the protocols.

I am not sure what the issue with mypy_primer, feel free to provide insights if you have any!

emmatyping · 2025-05-31T18:34:43Z

I am not sure what the issue with mypy_primer, feel free to provide insights if you have any!

Definitely looks unrelated. I searched through the codebase and there aren't any references to compression.zstd

srittau · 2025-06-02T10:48:42Z

The primer errors are a mypy hiccup. I've reopened the PR to trigger another run.

srittau

LGTM, let's see what primer says, but I don't expect any relevant output.

github-actions · 2025-06-02T11:02:00Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Rogdham added 2 commits May 24, 2025 13:00

Python 3.14: PEP-784 compression.zstd

219293a

Fix Self import

3ad00fc

This comment has been minimized.

Sign in to view

Rogdham added 2 commits May 24, 2025 13:35

Add to allowlist for 3.14 beta2

a37a648

Fix ZstdFile incompatibilities with parent class

ade5b67

This comment has been minimized.

Sign in to view

emmatyping reviewed May 24, 2025

View reviewed changes

Comment thread stdlib/_zstd.pyi Outdated

Rogdham added 2 commits May 24, 2025 19:08

Use Mapping[int, int] for options

b29dd87

Use reference for ZstdCompressor CONTINUE/FLUSH_BLOCK/FLUSH_FRAME

0f99105

This comment has been minimized.

Sign in to view

Merge branch 'origin/main'

901fecc

This comment has been minimized.

Sign in to view

Rogdham marked this pull request as ready for review May 29, 2025 07:41

srittau requested changes May 30, 2025

View reviewed changes

Explicit parameter defaults

f1e6f3d

Rogdham added 7 commits May 31, 2025 17:26

Explicit constant values

28975a0

Remove Any in set_parameter_types

c3cd99d

Change TextIO to TextIOWrapper

7177ce8

Avoid IO in file argument

b93c894

Remove redundancy in ZstdCompressor contants types

fdd5bbe

Add some more final/Final annotations

9def477

Remove Final from IntEnum members

94c7729

This comment has been minimized.

Sign in to view

srittau closed this Jun 2, 2025

srittau reopened this Jun 2, 2025

srittau approved these changes Jun 2, 2025

View reviewed changes

srittau merged commit 798f332 into python:main Jun 2, 2025

Rogdham deleted the pep-784_compression-zstd branch June 2, 2025 17:04

This was referenced Jun 7, 2025

Python 3.14: PEP-784 zstd in other modules #14225

Merged

Fix Protocol inheritance in _zstdfile #14232

Merged

yashwanthatla mentioned this pull request Jun 21, 2025

Replication of original PR #14129: Python 3.14: PEP-784 compression.zstd yashwanthatla/typeshed#3

Open

	def set_parameter_types(c_parameter_type: type[Any], d_parameter_type: type[Any]) -> None: ...
	def set_parameter_types(c_parameter_type: type[CompressionParameter], d_parameter_type: type[DecompressionParameter]) -> None: ...

		_PathOrFileBinary: TypeAlias = StrOrBytesPath \| IO[bytes]
		_PathOrFileText: TypeAlias = StrOrBytesPath \| IO[str]

Conversation

Rogdham commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Rogdham commented May 24, 2025

Uh oh!

emmatyping commented May 24, 2025

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Rogdham commented May 29, 2025

Uh oh!

srittau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rogdham commented May 31, 2025

Uh oh!

srittau commented May 31, 2025

Uh oh!

This comment has been minimized.

Rogdham commented May 31, 2025

Uh oh!

emmatyping commented May 31, 2025

Uh oh!

srittau commented Jun 2, 2025

Uh oh!

srittau left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rogdham commented May 24, 2025 •

edited

Loading