Skip to content
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

#75 Optipng fixes #77

Merged
merged 4 commits into from
Nov 21, 2023
Merged

#75 Optipng fixes #77

merged 4 commits into from
Nov 21, 2023

Conversation

Samreay
Copy link
Contributor

@Samreay Samreay commented Oct 18, 2023

Fixes #75

Initially just updated optipng to take the path instead of a str, but then noticed a few odd things in the code that I changed as well


# save other srcset paths, keyed by multiplication factor:
for mult in srcset_mult_facs:
if not (mult == 1):
if mult != 1:
Copy link
Owner

Choose a reason for hiding this comment

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

Same here - this is more readable of course, but maybe they had reasons to write the code that way in sphinx-gallery ?
Even if the original code is very strangely constructed IMHO, the less we modify it the more easily we can apply the same fixes than them.

Plus, since we did not port most of their tests yet in our code base (see #12), any modification is a risk of regression without CI safety harness.

@smarie
Copy link
Owner

smarie commented Nov 20, 2023

Hi @Samreay , thanks a lot for this PR !
I made a few comments above.
I am planning to cut a release in the upcoming days, maybe this could ship in ?
Thanks again

@Samreay
Copy link
Contributor Author

Samreay commented Nov 20, 2023

Let's see how this build goes

@smarie
Copy link
Owner

smarie commented Nov 21, 2023

It seems that the fix is not currently tested in the CI :

WARNING - optipng binaries not found, PNG images and thumbnails will not be optimized

https://github.com/smarie/mkdocs-gallery/actions/runs/6928047749/job/18843066689?pr=77#step:6:125

Unfortunately optipng cannot be installed by pip. We therefore have to modify the github actions workflow to install it.
I will now merge this "as is" and open another issue where we'll have to find a way to include optipng in the CI.

Thanks again !

@smarie smarie merged commit fe99820 into smarie:main Nov 21, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compress images errors because of string conversion when Path is required
2 participants