Skip to content

gh-132983: Add documentation for compression.zstd #133911

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

emmatyping
Copy link
Member

@emmatyping emmatyping commented May 12, 2025

@emmatyping emmatyping added the needs backport to 3.14 bugs and security fixes label May 12, 2025
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

syntactic review

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Some grammar notes.

Code block directives need a line after.

Please settle on gaps between descriptions for the file, currently it seems random, some parts use two and others one.

Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@AA-Turner
Copy link
Member

cc @python/proofreaders, I think the text here could benefit from a close reading, Stan has already noted several English grammar/syntax points.

@StanFromIreland
Copy link
Contributor

@python/proofreaders

Are there any active members, out of curiosity? I do not know of any nor have seen any around.

I think the text here could benefit from a close reading

I will go through it again later.

@bskinn
Copy link
Contributor

bskinn commented May 12, 2025

@python/proofreaders

Are there any active members, out of curiosity? I do not know of any nor have seen any around.

🙋

I at least look at all pings to the group. Will usually weigh in if I have time, and as long as no one else has beaten me to what I'd have to say.

Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@emmatyping
Copy link
Member Author

@StanFromIreland

Please settle on gaps between descriptions for the file, currently it seems random, some parts use two and others one.

I use one for items within a class, two for at the module level, but that's probably too complicated and I should probably just choose two lines.

@mwichmann
Copy link

@python/proofreaders

Are there any active members, out of curiosity? I do not know of any nor have seen any around.

There are, but we don't get explicitly called on very often, it seems...


.. module:: compression.zstd
:synopsis: Low level interface to compression and decompression routines in
Meta's zstd library

Choose a reason for hiding this comment

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

Wouldn't call out Meta here. The reference manual for the zstd itself doesn't mention a company name. On the other hand, it might not be a bad idea to reference that manual somewhere (https://facebook.github.io/zstd/zstd_manual.html).

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Meta's zstd library
the Zstandard library

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, the library is called zstd, perhaps we should say "in the zstd library"?

When opening a file for reading, the *options* argument can be a dictionary
providing advanced decompression parameters, see
:class:`DecompressionParameter` for detailed information about supported
parameters. The *zstd_dict* argument is a :class:`ZstdDict` instance to be

Choose a reason for hiding this comment

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

Couldn't lines 59-62, 66-69, 102-105 and 109-112 be combined into a single clause, leaving only the specifics for reading and writing as unique? At the very least, four of these clauses in a short space seems a lot, two may be okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I think the line numbers have changed and Github does not do a good job at showing enough context for your comment, what were you proposing?


.. attention::

The :mod:`!compression` package is the new location for the data compression

Choose a reason for hiding this comment

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

"New" always seems such a temporal term. I'm just spitballing here, but how about something like: "The compression package provides a unified interface to several data compression routines. Some of these have historically been available as separate modules; those will continue to be available under their original names for compatibility reasons, and will not be removed without a deprecation cycle".

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I definitely understand "new" may not be the right word here. I worry if we say "a unified interface to ... compression routines" that people will expect compression to hold some high level compress function that abstracts over the various implementations, which is not the case.

Perhaps
"The compression package contains the canonical compression modules containing interfaces to several different algorithms. Some of these have historically been available as separate modules; those will continue to be available under their original names for compatibility reasons, and will not be removed without a deprecation cycle. The compression.* import names are encouraged for use where practical."


.. module:: compression.zstd
:synopsis: Low level interface to compression and decompression routines in
Meta's zstd library
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Meta's zstd library
the Zstandard library

emmatyping and others added 3 commits May 13, 2025 18:40
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
- Rewrite the digested/undigested dict section
- Add a section for exceptions
- wrap lines to ~80 chars
- clarify that an exception is raised if level is passed to a decompressor
- make quotes in docs for open vs ZstdFile consistent
- Remove currently and repeated "note"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir needs backport to 3.14 bugs and security fixes skip news
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

6 participants