Include Cocaine error when there was an error processing thumbnails #2415
Conversation
spec/paperclip/thumbnail_spec.rb
Outdated
@thumb.make | ||
end | ||
}.to raise_error Paperclip::Error, /unrecognized option `-this-aint-no-option'/ |
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.
Line is too long. [91/80]
assert_raises(Paperclip::Error) do | ||
silence_stream(STDERR) do | ||
silence_stream(STDERR) do | ||
expect { |
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.
Avoid using {...} for multi-line blocks.
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.
Hmm. A continual conundrum for me.
expect do
blah
end.to raise_error
I think I prefer {...}
, but what's your opinion?
lib/paperclip/thumbnail.rb
Outdated
@@ -78,7 +78,10 @@ def make | |||
|
|||
success = convert(parameters, :source => "#{File.expand_path(src.path)}#{'[0]' unless animated?}", :dest => File.expand_path(dst.path)) | |||
rescue Cocaine::ExitStatusError => e | |||
raise Paperclip::Error, "There was an error processing the thumbnail for #{@basename}" if @whiny | |||
if @whiny | |||
message = "There was an error processing the thumbnail for #{@basename}:\n" + e.message |
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.
Line is too long. [97/80]
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.
I think I prefer a long line here, but I'm happy to split it.
assert_raises(Paperclip::Error) do | ||
silence_stream(STDERR) do | ||
silence_stream(STDERR) do | ||
expect { |
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.
Style/BlockDelimiters: Avoid using {...} for multi-line blocks.
@@ -85,7 +85,10 @@ def make | |||
dest: File.expand_path(dst.path), | |||
) | |||
rescue Terrapin::ExitStatusError => e | |||
raise Paperclip::Error, "There was an error processing the thumbnail for #{@basename}" if @whiny | |||
if @whiny | |||
message = "There was an error processing the thumbnail for #{@basename}:\n" + e.message |
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.
Metrics/LineLength: Line is too long. [97/80]
We have been occasionally seeing errors in production telling us "There was an error processing the thumbnail". Unfortunately, Paperclip doesn't currently tell us what was ImageMagick was complaining about, so we weren't able to debug further what was going on.
This PR ensures that the original error message from Cocaine (and therefore ImageMagick) is included in the Paperclip error (when
whiny
mode is on), which allowed us to identify the problem with our ImageMagick configuration.