Skip to content
This repository was archived by the owner on Feb 18, 2021. It is now read-only.

Moved from crop to extent for filling images #34

Merged
merged 1 commit into from
Mar 17, 2016

Conversation

twolfson
Copy link
Contributor

In #32, we received a report of 2 images failing to properly compare. We resolved that the issue was due to us improperly using crop which lead to 1 error and 1 of the images had a poor encoding. The combination of these 2 lead gm to create 1x1 images to diff.

Reference images here: #33 (comment)

To remedy this, we are moving off of the improper command crop (which truncates images) to the more accurate extent (which extends images). In this PR:

For the release verison, please release as 1.4.0:

./release.sh 1.4.0

/cc @mlmorg

@twolfson twolfson force-pushed the dev/use.extent.sqwished branch from ef3aa9f to f2ad6d7 Compare March 16, 2016 05:36
function transparentExtent(gm, params) {
// Assert we received our parameters
assert.notEqual(params.width, undefined);
assert.notEqual(params.height, undefined);
Copy link

Choose a reason for hiding this comment

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

Why not more readable error messages? These parameters are read in using input from the user (eventually) so there's a chance the consuming code could have erroneous input. An assert doesn't really explain the issue all that well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These assertions are for preventing typos from developers. There's not really a case where the error would come from user error:

Copy link

Choose a reason for hiding this comment

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

👍 gotcha

mlmorg added a commit that referenced this pull request Mar 17, 2016
Moved from crop to extent for filling images
@mlmorg mlmorg merged commit 2d95027 into uber-archive:master Mar 17, 2016
@x3au
Copy link

x3au commented Mar 17, 2016

Thanks for the fix!

@twolfson twolfson deleted the dev/use.extent.sqwished branch May 11, 2016 05:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diff failing on Windows
4 participants