[WIP] convert and optimize image in a single pass#289
[WIP] convert and optimize image in a single pass#289cking100 wants to merge 1 commit intoopenzim:mainfrom
Conversation
|
I've converted PR to draft since it is not ready, please mark it as ready once ... ready 🤣 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
==========================================
- Coverage 99.52% 97.32% -2.20%
==========================================
Files 41 41
Lines 2516 2584 +68
Branches 354 383 +29
==========================================
+ Hits 2504 2515 +11
- Misses 8 51 +43
- Partials 4 18 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e5357ec to
c10b2eb
Compare
|
@benoit74 I will add the test cases soon. But, before that, can I please get a review on the changes? |
benoit74
left a comment
There was a problem hiding this comment.
Few remarks. You are on the right path, thank you
631d2e7 to
b0eea3c
Compare
|
@benoit74 I have made the requested changes. Can you please take a look? Thanks |
benoit74
left a comment
There was a problem hiding this comment.
Nice, I realize we still have "details" to sort out ^^
Another general question: do we really need the @overload definitions now? I don't get anymore what they are adding in this specific case since it looks to me we now support all combinations (src is Path and dst is BytesIo, src is BytesIO and dst is Path, ...)
| delete_src: whether to remove src file upon success (boolean) | ||
| values: True | False | ||
| convert: whether/how to convert from source before optimizing (str or boolean) | ||
| convert: whether to convert from source before optimizing (boolean) |
There was a problem hiding this comment.
Sorry, I just realize now that dst_format was expected to be in convert variable.
I kinda prefer the new API which is more self-explanatory than "hiding" the dst_format in convert variable.
I also feel like the convert option is even not needed at all. We can automatically decide when conversion is needed based on src and dst format (either passed explicity or guessed from file names).
But this is a breaking API change, which would require a major release. I don't think we need it now, but we can prepare for it:
- still support convert to contain both bool and dst_format (in addition to dst_format variable)
- if something is passed in convert act accordingly (it "wins") but indicate it is a deprecated way of calling the API, expected to be remove in scraperlib 6
- prepare another issue / PR to remove the
convertparameter
WDYT?
There was a problem hiding this comment.
Good Idea. For now, we should support both convert variable as str as well as dst_format for backward compatibility, but raise a deprecated warning as you said, and we can prepare and remove(in future) the convert parameter directly since it's not needed anymore.
|
Regarding the Overloads, since these functions are used by multiple scrapers, you might know better than me. We can either remove overloads and have cleaner code, but then we will have to deal with union type every time the functions are being called, or keep them and don't worry about it. @benoit74 |
Let keep it as-is for now and I will double-check this later |
b0eea3c to
bb3e1c6
Compare
Fixes #284
Changes:
optimize_xxxfunctions to accept aconvertparameteroptimize_imageandoptimize_gifto acceptio.BytesIOas inputbytesinsrcoptimize_xxxnow works on single passTODO