Skip to content

Conversation

gurgeous
Copy link
Contributor

This is not done yet, but I thought you might want to take a look. Maybe something like this? See sample output below. A few questions:

  • should I add tests? not sure if that's useful here, let me know
  • I could try to load/use TableTennis if available, without adding a dependency.. any interest?
  • want me to modify the changelog? anything else?

Thanks!!

$ image_optim --benchmark -r /tmp/corpus/

benchmarking: 100.0% (elapsed: 3.9m)

BENCHMARK RESULTS

name      files  elapsed  kb saved     kb/s
--------  -----  -------  --------  -------
oxipng       50    8.906  1867.253  209.664
pngquant     50    1.980   214.597  108.386
pngcrush     50   22.529  1753.704   77.841
optipng      50  142.940  1641.101   11.481
advpng       50  137.753   962.549    6.987
pngout       50  426.706   444.679    1.042

@gurgeous
Copy link
Contributor Author

One rubocop issue remaining:

lib/image_optim.rb:28:1: C: Metrics/ClassLength: Class has too many lines. [170/150]
class ImageOptim ...
^^^^^^^^^^^^^^^^

There are many ways to fix this, but I always try to avoid refactors in other people's projects :) Happy to address this if you give me some direction.

Copy link
Owner

@toy toy left a comment

Choose a reason for hiding this comment

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

Sorry for the wait. Works well, most important comments are for lib/image_optim.rb, everything else is code style and nitpicks.

About tests - better to know if it will break, so at least on the level of that it works.

About TableTennis - I understand the desire to make it beautiful, but the output is so simple, that it feels excessive. From one side it shouldn't add more than few lines of code, from the other there are lots of different gems to print a table in terminal and everyone may have their preferred one.

About changelog entry - definitely and some mention in readme

Comment on lines 174 to 177
dst = src.temp_path
begin
begin
worker.optimize(src, dst, timeout: timer)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if applying worker always to the original is more informative then doing it in chain (on result of previous worker) as will be done during normal operation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can add a --chain later if people like this. I think those results would be interesting as well, especially now that I've seen the worker_analysis script

@gurgeous
Copy link
Contributor Author

gurgeous commented Sep 3, 2025

Thanks for the feedback! I added a simple test and applied the feedback. I think this is ready for another look now, no rush

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.

2 participants