Skip to content

gh-138848: Add PNG optimization to pre-commit#139061

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

Closed
StanFromIreland wants to merge 4 commits into from
Closed

gh-138848: Add PNG optimization to pre-commit #139061

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c214244
Commit
StanFromIreland Sep 17, 2025
c2506ce
Commit
StanFromIreland Sep 17, 2025
2ee9bf9
Apply suggestions from code review
StanFromIreland Sep 29, 2025
db82a4b
Move & add comment
StanFromIreland Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -84,6 +84,33 @@ repos:
hooks:
- id: zizmor

- repo: https://github.com/oxipng/oxipng
rev: v9.1.5
hooks:
- id: oxipng
args: [--strip=safe, --alpha]
# Exclude all existing pngs

@hugovkhugovkSep 29, 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.

Is it all existing pngs, or a subset?

@StanFromIrelandStanFromIrelandSep 29, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

All, however, it would just save off a byte or two for a few.

@zwarezwareSep 29, 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 really don't think we want to exclude all existing images, that will severely reduce the usefulness of the new check. After all, the most likely changes are going to be to existing images to update them.

Does anyone know why we use --all-files in the CI pre-commit check? Surely it would be better to make use of --from-ref and --to-ref?

@zwarezwareSep 29, 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.

Alternately, we should just take the hit once to enable the check and optimize the snot out of everything in this PR with no exclude list.

@AA-TurnerAA-TurnerSep 29, 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.

Optimising exisiting PNGs has some benefit, but results in a larger repo size. The only user-visible effects should be in the documentation, where we'd preferably use SVGs to PNGs (especially for diagrams etc).

Personally, I think this check would probably cost more than it saves (at least on a macro scale), but I don't have a strong opinion, I don't use pre-commit.

exclude: |
Comment thread
mhsmith marked this conversation as resolved.
(?x)^(
Android/testbed/app/src/main/res/drawable-xxhdpi/ic_launcher\.png|
Doc/howto/logging_flow\.png|
Doc/library/hashlib-blake2-tree\.png|
Doc/library/kde_example\.png|
Doc/library/pathlib-inheritance\.png|
Doc/library/tk_msg\.png|
Doc/library/turtle-star\.png|
Doc/using/mac_installer_[^/]*\.png|
Doc/using/win_install[^/]*\.png|
InternalDocs/images/python-cyclic-gc-[^/]*-new-page\.png|
Lib/idlelib/Icons/idle_[^/]*\.png|
Lib/profiling/sampling/_assets/python-logo-only\.png|
Lib/test/test_email/data/python\.png|
Lib/test/tkinterdata/python\.png|
Objects/object_layout_[^/]*\.png|
PC/icons/[^/]*\.png|
Tools/msi/bundle/SideBar\.png|
)$

- repo: https://github.com/sphinx-contrib/sphinx-lint
rev: v1.0.0
hooks:
Expand Down
Loading