-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Do not recode JPEG files in imagepipelines #4753
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4753 +/- ##
==========================================
+ Coverage 88.67% 88.87% +0.19%
==========================================
Files 162 162
Lines 11013 11026 +13
Branches 1802 1808 +6
==========================================
+ Hits 9766 9799 +33
+ Misses 966 947 -19
+ Partials 281 280 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice take over!
The coverage data shows an issue, though: the tests only cover the change in convert_image
, the change in get_images
, which is the most significant, is not currently covered by tests for scenarios where the new parameter is missing.
Hey @Gallaecio, I think the test cases cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Ummm @kmike you requested some changes in the previous PR. This looks good to go now? |
The implementation looks good to me overall @drs-11, thanks for picking it up! I haven't really checked the tests, but rely on you all to get them right :) +1 to merge after addressing a couple of comments about warning messages. |
This patch completes the last few suggestions on the pull request #3689.
Fixes #3055, closes #3689