Skip to content

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

Merged
gpshead merged 23 commits into
python:mainfrom
emmatyping:zstd-docs
May 21, 2025
Merged

gh-132983: Add documentation for compression.zstd#133911
gpshead merged 23 commits into
python:mainfrom
emmatyping:zstd-docs

Conversation

@emmatyping

@emmatyping emmatyping commented May 12, 2025 •
edited by hugovk
Loading

Copy link
Copy Markdown
Member

@bedevere-app bedevere-app Bot added docs Documentation in the Doc dir skip news labels May 12, 2025
@bedevere-app bedevere-app Bot mentioned this pull request May 12, 2025
Closed
13 tasks done
@github-project-automation github-project-automation Bot moved this to Todo in Docs PRs May 12, 2025
@emmatyping
emmatyping requested review from AA-Turner and gpshead May 12, 2025 02:48
@emmatyping emmatyping added the needs backport to 3.14 bugs and security fixes label May 12, 2025
AA-Turner
AA-Turner reviewed May 12, 2025
View reviewed changes

ghost left a comment

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.

syntactic review

Comment thread Doc/library/archiving.rst Outdated
Comment thread Doc/library/compression.rst Outdated
Comment thread Doc/library/compression.rst Outdated
Comment thread Doc/library/compression.rst
Comment thread Doc/library/compression.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Emma Smith Adam Turner
Apply suggestions from AA-Turner
cfe0590 
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
StanFromIreland
StanFromIreland reviewed May 12, 2025
View reviewed changes

ghost left a comment

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.

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.

Comment thread Doc/library/compression.rst
Comment thread Doc/library/compression.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Emma Smith Adam Turner Stan Ulbrych
Apply suggestions from reviewers
5115b4c 
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

ghost commented May 12, 2025

Copy link
Copy Markdown
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

ghost commented May 12, 2025

Copy link
Copy Markdown
Member

@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.

StanFromIreland
StanFromIreland reviewed May 12, 2025
View reviewed changes
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
@bskinn

ghost commented May 12, 2025

Copy link
Copy Markdown
Contributor

@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.

Emma Smith Stan Ulbrych
Apply suggestions from reviewers
4ab7fd7 
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
@emmatyping

ghost commented May 12, 2025

Copy link
Copy Markdown
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.

StanFromIreland
StanFromIreland reviewed May 12, 2025
View reviewed changes
Comment thread Doc/library/compression.zstd.rst Outdated
@mwichmann

ghost commented May 12, 2025

Copy link
Copy Markdown

@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...

mwichmann
mwichmann reviewed May 12, 2025
View reviewed changes
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.rst Outdated
Comment thread Doc/library/compression.rst Outdated
hugovk
hugovk reviewed May 13, 2025
View reviewed changes
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Emma Smith and others added 3 commits May 13, 2025 18:40
Emma Smith Hugo van Kemenade Stan Ulbrych
Apply suggestions from reviewers
5eb5efc 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
Emma Smith Hugo van Kemenade
Don't reference self when referring to items
987bd27 
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Emma Harper Smith
Updates to respond to review
615ed7f 
- 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"
StanFromIreland
StanFromIreland reviewed May 18, 2025
View reviewed changes

ghost left a comment

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.

Looking pretty good now.

Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
brainwane
brainwane reviewed May 19, 2025
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't absolutely thorough in reviewing all the proposed text, but have a few suggestions for clarification. Thanks for the work.

Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Emma Smith Stan Ulbrych Sumana Harihareswara
Apply suggestions from Sumana and Stan
e61e9a1 
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
Co-authored-by: Sumana Harihareswara <sh@changeset.nyc>
@StanFromIreland StanFromIreland mentioned this pull request May 19, 2025
Merged
vadmium
vadmium reviewed May 20, 2025
View reviewed changes
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Emma Harper Smith and others added 5 commits May 20, 2025 12:01
Emma Harper Smith
Remove ref to Meta and clean up mode usage
1149832
Emma Smith Martin Panter
Apply suggestions from vadmium
71ed7c3 
Co-authored-by: Martin Panter <vadmium@users.noreply.github.com>
Emma Harper Smith
Many updates to respond to review
2f895dd 
- improve argument descriptions
- improve clarity about the meaning of parameter value 0s
- clarify behavior of compress/flush
- many more clarifications/wording improvements
Emma Harper Smith
Add examples to (De)compressionParameter
f25e6e7
Emma Harper Smith
Add reference to zstd manual and blurb on algorithm
9ff6320
serhiy-storchaka
serhiy-storchaka approved these changes May 21, 2025
View reviewed changes

ghost left a comment

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.

Besides some nitpicks, LGTM.

Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst
decompression, passing a digested dictionary will reduce the overhead of
loading the dictionary.

.. list-table:: Difference for compression

ghost May 21, 2025

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.

I think that simple nested item lists would look better here. Maybe not even nested item lists, just an item lists with two or three paragraphs in every item.

ghost May 21, 2025 •
edited by emmatyping
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.

Sorry, I'm not sure I understand what you have in mind, perhaps it can be resolved in a follow-up?

Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Comment thread Doc/library/compression.zstd.rst Outdated
Emma Harper Smith added 2 commits May 21, 2025 09:59
Emma Harper Smith
Expand on the connection between level and compression_level
daa9df1
Emma Harper Smith
Resolve review suggestions
b3fd3cd 
- Use "integer" instead of "int object"
- Use > 0 rather than >=1 for nb_workers
@gpshead gpshead moved this from Todo to Done in Docs PRs May 21, 2025
@gpshead
gpshead merged commit d862b6d into python:main May 21, 2025
@miss-islington-app

ghost commented May 21, 2025

Copy link
Copy Markdown

Thanks @emmatyping for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@bedevere-app

ghost commented May 21, 2025

Copy link
Copy Markdown

GH-134425 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label May 21, 2025
@emmatyping
emmatyping deleted the zstd-docs branch May 21, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vadmium vadmium vadmium left review comments
@AA-Turner AA-Turner AA-Turner left review comments
@StanFromIreland StanFromIreland StanFromIreland left review comments
@gpshead gpshead gpshead approved these changes
@hugovk hugovk hugovk approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
+2 more reviewers
@brainwane brainwane brainwane left review comments
@mwichmann mwichmann mwichmann left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

Docs PRs
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@emmatyping @AA-Turner @StanFromIreland @bskinn @mwichmann @gpshead @brainwane @vadmium @hugovk @serhiy-storchaka