GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-132983: Minor fixes and clean up for the _zstd module by serhiy-storchaka · Pull Request #134930 · python/cpython

Conversation

serhiy-storchaka commented May 30, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Implement PEP 784 - Adding Zstandard to the Python standard library #132983

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-132983: Minor fixes and clean up for the _zstd module

9aec708

serhiy-storchaka requested review from AA-Turner and emmatyping

May 30, 2025 14:22

serhiy-storchaka added skip news needs backport to 3.14 bugs and security fixes labels

May 30, 2025

bedevere-app Bot added the awaiting core review label

May 30, 2025

bedevere-app Bot mentioned this pull request

May 30, 2025

Implement PEP 784 - Adding Zstandard to the Python standard library #132983

Closed

13 tasks done

emmatyping reviewed

May 30, 2025

View reviewed changes

emmatyping left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

One question and one concern, but the rest looks good! Thank you for the improvements :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_zstd/compressor.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_zstd/decompressor.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner reviewed

May 31, 2025

View reviewed changes

Comment thread

Modules/_zstd/_zstdmodule.c

Comment on lines -510 to -515

		if (!PyType_Check(c_parameter_type) \|\| !PyType_Check(d_parameter_type)) {
		PyErr_SetString(PyExc_ValueError,
		"The two arguments should be CompressionParameter and "
		"DecompressionParameter types.");
		return NULL;
		}

AA-Turner May 31, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sanity check on removing the type checks here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

emmatyping May 31, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The type checks are already handled by argument clinic, so these are actually redundant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Modules/_zstd/compressor.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_zstd/decompressor.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka and others added 6 commits

May 31, 2025 21:56


          Remove defensive double checks after releasing the GIL.

0251f9e


          Apply suggestions from code review

710a2b3

Co-authored-by: Adam Turner <[email protected]>


          Merge remote-tracking branch 'refs/remotes/origin/zstd-fixes' into zs…

88e9c0a

…td-fixes


          Refactor _zstd_load_c_dict and _zstd_load_d_dict.

cffe18b

ts

0dd3b2a


          Merge branch 'main' into zstd-fixes

4877c81

emmatyping approved these changes

May 31, 2025

View reviewed changes

emmatyping left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The changes look good! One minor wording suggestion but great otherwise. Thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_zstd/_zstdmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka and others added 2 commits

June 1, 2025 09:04


          Update Modules/_zstd/_zstdmodule.c

b15f2e4

Co-authored-by: Emma Smith <[email protected]>


          Update tests.

fe83254

serhiy-storchaka merged commit b595237 into python:main

Jun 1, 2025

bedevere-app Bot removed the awaiting core review label

Jun 1, 2025

miss-islington-app Bot commented Jun 1, 2025

Copy link

Copy Markdown

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jun 1, 2025


          pythongh-132983: Minor fixes and clean up for the _zstd module (pytho…

7275ec4

…nGH-134930)

(cherry picked from commit b595237)

Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-app Bot commented Jun 1, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label

Jun 1, 2025

serhiy-storchaka commented Jun 1, 2025

Copy link

Copy Markdown

Member Author

Thank you @AA-Turner and @emmatyping for your review.

emmatyping reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka deleted the zstd-fixes branch

June 1, 2025 08:23

serhiy-storchaka added a commit that referenced this pull request

Jun 1, 2025


          [3.14] gh-132983: Minor fixes and clean up for the _zstd module (GH-1…

035c4d6

…34930) (GH-134998)

(cherry picked from commit b595237)

Co-authored-by: Serhiy Storchaka <[email protected]>

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request

Jul 12, 2025


          pythongh-132983: Minor fixes and clean up for the _zstd module (pytho…

7be1b25

…nGH-134930)

taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request

Aug 4, 2025


          pythongh-132983: Minor fixes and clean up for the _zstd module (pytho…

38e3634

…nGH-134930)

Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request

Aug 19, 2025


          pythongh-132983: Minor fixes and clean up for the _zstd module (pytho…

9acbfcc

…nGH-134930)

Reviewers

AA-Turner

AA-Turner left review comments

emmatyping

emmatyping approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

serhiy-storchaka commented May 30, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emmatyping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AA-Turner May 31, 2025

Choose a reason for hiding this comment

Uh oh!

emmatyping May 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emmatyping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miss-islington-app Bot commented Jun 1, 2025

Uh oh!

bedevere-app Bot commented Jun 1, 2025

Uh oh!

serhiy-storchaka commented Jun 1, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-storchaka commented May 30, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.