-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added resolution unit option in tiff method #3023
Conversation
ompal-sisodiya
commented
Dec 23, 2021
- Added resolution unit in option.param while using tiff method
- Added required test cases
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.
Thank you Ompal for the PR, looks good. Please can you add a unit test that verifies the EXIF metadata in the output has the expected resolution unit based on this new option.
Sorry, I should have been clearer, I was thinking of a unit test that verifies the EXIF metadata of the output image (rather than just inspecting internal options), perhaps using an approach similar to the existing metadata tests: Lines 53 to 56 in 1ff84b2
|
@lovell All the required change are done on our end. could you review once. |
This comment has been minimized.
This comment has been minimized.
@ompal-sisodiya Thanks for the updates. I don't see any verification of output image EXIF metadata in the newly-added tests; am I missing something? (I note you've added to an existing unit test, but that logic is unrelated to this PR, I was merely using it as an example of using |
@lovell Thanks for your response. please review the below test for the verification of output image with EXIF metadata.
|
@ompal-sisodiya Please can you add an assertion on the value of |
@lovell I have added assertion for the value of resolution unit. could you review once. |
This comment has been minimized.
This comment has been minimized.
@lovell Could please review PR? |
Thanks again for taking the time to submit a PR, I will review this when I am ready to do so. Coordinated pestering won't make that happen any sooner. |
Co-authored-by: Lovell Fuller <[email protected]>
Landed via commit f7bed69 I also added the (normalised) This will be in v0.30.0, thanks again for the PR. |
Co-authored-by: Lovell Fuller <[email protected]>
Co-authored-by: Lovell Fuller <[email protected]>