Add failOnError to suit #976
Conversation
|
The linux builds are passing now. Mac builds fail in the same fashion as your other suit branch builds, like https://travis-ci.org/lovell/sharp/builds/282365910, so I think this PR is ready to merge. |
There was a problem hiding this comment.
This is looking great, thank you Matthew. I've left a couple of comments inline; nothing serious.
As you noticed, the OS X binaries for the pre-release libvips v8.6.0 are not yet ready so no need to worry about that platform at the moment.
The FALSE thing you mention is probably a case thing, which then made me realise that the existing C++ code should be using false everywhere instead. This is probably a hangover from when sharp was using the libvips' C API. I'll update that separately after this PR lands.
lib/input.js
Outdated
| @@ -26,6 +26,15 @@ function _createInputDescriptor (input, inputOptions, containerOptions) { | |||
| throw new Error('Unsupported input ' + typeof input); | |||
| } | |||
| if (is.object(inputOptions)) { | |||
| // Fail on error | |||
| inputDescriptor.failOnError = false; | |||
There was a problem hiding this comment.
We could hoist the default to the beginning of this function, something like:
const inputDescriptor = { failOnError: false };
src/common.cc
Outdated
| @@ -52,6 +52,9 @@ namespace sharp { | |||
| descriptor->buffer = node::Buffer::Data(buffer); | |||
| buffersToPersist.push_back(buffer); | |||
| } | |||
| if (HasAttr(input, "failOnError")) { | |||
There was a problem hiding this comment.
Always having failOnError set in JS-land means we can remove a conditional here and always set it on the descriptor struct.
src/common.cc
Outdated
| @@ -220,6 +223,9 @@ namespace sharp { | |||
| if (imageType != ImageType::UNKNOWN) { | |||
| try { | |||
| vips::VOption *option = VImage::option()->set("access", accessMethod); | |||
| if (descriptor->failOnError) { | |||
There was a problem hiding this comment.
Ditto here, something like:
vips::VOption *option = VImage::option()
->set("access", accessMethod)
->set("fail", descriptor->failOnError);There was a problem hiding this comment.
Should line 262 mirror the logic on line 223?
|
|
||
| it('returns errors to callback for truncated JPEG when failOnError is set', function (done) { | ||
| sharp(fixtures.inputJpgTruncated, { failOnError: true }).toBuffer(function (err, data, info) { | ||
| assert.ok(err.message.includes('VipsJpeg: Premature end of JPEG file'), err); |
There was a problem hiding this comment.
I really like this. assert(err.message.includes( is highly fluent to read. 💯
| it('handles truncated JPEG by default', function (done) { | ||
| sharp(fixtures.inputJpgTruncated) | ||
| .resize(320, 240) | ||
| // .toFile(fixtures.expected('truncated.jpg'), done); |
There was a problem hiding this comment.
Looks like there are a couple of leftover comments in here.
There was a problem hiding this comment.
those comments were left in so Engineers of Tomorrow can easily re-create the truncated images (in case VIPS rendering of truncated images changes).
Do you still want me to remove them?
|
I added a temporary commit 50563c4 so this PR will pass CI after the next rebase+push. |
src/pipeline.cc
Outdated
| @@ -250,6 +250,9 @@ class PipelineWorker : public Nan::AsyncWorker { | |||
| if (shrink_on_load > 1) { | |||
| // Reload input using shrink-on-load | |||
| vips::VOption *option = VImage::option()->set("shrink", shrink_on_load); | |||
| if (baton->input->failOnError) { | |||
There was a problem hiding this comment.
Similar question here, should the logic match? Perhaps move it into a helper? (I notice accessMethod is missing here - that looks like an existing bug though so I'll sort it later.)
There was a problem hiding this comment.
I'm not going to add a helper, but I'll push this change.
|
Marvellous, thank you for all your work on this Matthew. |
|
Thanks for sharp! |
suitfrom Add failOnError #925option->set("fail", FALSE)still set the flag (!!), so that flag is set conditionally in this branch.