Skip to content

gh-132983: Style improvements for compression.zstd #133547

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
May 14, 2025
Merged

gh-132983: Style improvements for compression.zstd #133547

merged 10 commits into from
May 14, 2025
+62 −61

Conversation

emmatyping
Copy link
Member

@emmatyping emmatyping commented May 7, 2025 •
edited
Loading

Mostly just make both files consistently use single vs double quotes and add spacing as suggested in PEP 8.

  • consistently uses single vs double quotes per PEP 8
  • makes spacing consistent per PEP 8
  • adds a missing space in a multi-line string
  • Moves compression packages that don't have multiple files to modules
  • Adds a missing trailing comma in tarfile

@emmatyping
Format compression.zstd to match PEP 8
526445f 
Mostly just make both files consistently use single vs double quotes.
Also add a missing space in a multi-line string.
@bedevere-app bedevere-app bot mentioned this pull request May 7, 2025
Open
11 tasks
@emmatyping
Support finding libzstd not in pkg-config
a5c20a8 
pythonGH-133479 removed the logic to check for libzstd outside of pkg-config.
This commit adds that logic back with a check for the version so that
users can provide their own libzstd. This is to bring parity with lzma,
bz2, and zlib detection.
@emmatyping emmatyping requested review from AA-Turner and gpshead May 7, 2025 03:23
Rogdham
Rogdham approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@Rogdham Rogdham left a comment

Choose a reason for hiding this comment

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

I am confirming you did not miss any ', except maybe in the docstring of ZstdFile.readline which mentions b''

@emmatyping
Copy link
Member Author

I am confirming you did not miss any ', except maybe in the docstring of ZstdFile.readline which mentions b''

Yeah I left that one intentionally as docs seem to use b'' when talking about empty bytes objects.

@emmatyping
Copy link
Member Author

@AA-Turner @gpshead pinging just to follow up on the request for review on this one.

@emmatyping emmatyping mentioned this pull request May 13, 2025
Merged
@merwok
Copy link
Member

merwok commented May 13, 2025

I would argue that the (C)Python house style is to use single quotes, as that matches the output of repr and thus the REPL.

Formatters like black took position for double quotes for various reasons (I think arguing that it’s more common to have apostrophes inside the quotes, and something about qwerty layout), but that’s not universally liked.

So this change could be done (the files already have a non-small git history!), or not done.

@merwok
Copy link
Member

merwok commented May 13, 2025

Bringing here a suggestion from a closed PR: avoid unnecessary packages for the re-exported compression sub-modules (rename Lib/compression/bzip2/__init__.py → Lib/compression/bzip2.py)

@emmatyping emmatyping requested a review from ethanfurman as a code owner May 13, 2025 23:52
@emmatyping emmatyping removed the request for review from ethanfurman May 13, 2025 23:53
@emmatyping
Copy link
Member Author

Moved the modules and changed to single quotes! I also added a trailing comma that @merwok noted in an already-merged PR. I also updated the PR description to cover these points.

@emmatyping emmatyping requested a review from merwok May 14, 2025 00:21
merwok
merwok reviewed May 14, 2025
View reviewed changes
AA-Turner
AA-Turner reviewed May 14, 2025
View reviewed changes
@AA-Turner AA-Turner added the needs backport to 3.14 bugs and security fixes label May 14, 2025
@AA-Turner AA-Turner enabled auto-merge (squash) May 14, 2025 10:53
@AA-Turner AA-Turner merged commit b44c824 into python:main May 14, 2025
44 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 14, 2025
@emmatyping @AA-Turner @miss-islington
pythongh-132983: Style improvements for compression.zstd (pythonGH-…
cfbea08 
…133547)

(cherry picked from commit b44c824)

Co-authored-by: Emma Smith <emma@emmatyping.dev>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented May 14, 2025

GH-134001 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 14, 2025
AA-Turner added a commit that referenced this pull request May 14, 2025
@miss-islington @emmatyping @AA-Turner
[3.14] gh-132983: Style improvements for compression.zstd (GH-133547)…
a962934 
… (#134001)

gh-132983: Style improvements for `compression.zstd` (GH-133547)
(cherry picked from commit b44c824)

Co-authored-by: Emma Smith <emma@emmatyping.dev>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merwok merwok merwok left review comments

@Rogdham Rogdham Rogdham approved these changes

@AA-Turner AA-Turner AA-Turner approved these changes

@gpshead gpshead Awaiting requested review from gpshead

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emmatyping @merwok @Rogdham @AA-Turner