Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Delegate #readbyte on the abstract IO adapter, and improve documentation around custom processors #2034

Merged
merged 1 commit into from
Aug 5, 2016

Conversation

creature
Copy link
Contributor

I recently implemented a custom processor that used the Exifr gem to extract EXIF information from images uploaded to a Paperclip attachment. Exifr's processor uses readbyte to parse the EXIF header, so it hit errors when Paperclip's File-like object didn't have one. There's also a test for this delegation.

I've also tidied up the README documentation for custom processors, hopefully to be more clear than before. There was some duplicated content between the "Post Processing" section and the "Custom Attachment Processing" section, and those sections were separated in the file. I've dedicated the "Post Processing" section to Paperclip's built in thumbnailing processors, and made "Custom Attachment Processing" section solely about writing & using your own custom processors. This should be a more understandable progression, as built-in functionality is discussed first & separately from extending Paperclip.

I've also tidied up a couple of inaccuracies in the Paperclip::Processor documentation, and generally applied Strunk & White's "Omit needless words" principle in the areas I've touched.

@@ -34,7 +34,7 @@ def content_type
@adapter.tempfile = stub("Tempfile")
end

[:binmode, :binmode?, :close, :close!, :closed?, :eof?, :path, :rewind, :unlink].each do |method|
[:binmode, :binmode?, :close, :close!, :closed?, :eof?, :path, :readbyte, :rewind, :unlink].each do |method|

Choose a reason for hiding this comment

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

Line is too long. [112/80]

@creature
Copy link
Contributor Author

creature commented Nov 4, 2015

Are there any changes or amendments you'd like to see to this? I'm happy to tweak it as required to get this merged.

@tute
Copy link
Contributor

tute commented May 9, 2016

Thank you, @creature! This is great. Can you please rebase on top of latest master, and squash your commits together? Also, if you can, please add your good PR description as commit message. (Check https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history if needed).

Can you add context on the :readbyte addition? Thanks, again! 😃

@creature
Copy link
Contributor Author

Hi! Sorry for the slow reply. I'll try to rebase & squash the commits as you requested some time soon.

As for context on readbyte, it's a method defined on Ruby's IO class, and File < IO, so that's why Exifr presumes it will be available. Is that what you had in mind, or am I off-base? :)

…f custom processors.

I recently implemented a custom processor that used the Exifr gem to
extract EXIF information from images uploaded to a Paperclip attachment.
Exifr's processor uses readbyte to parse the EXIF header, so it hit
errors when Paperclip's File-like object didn't have one. There's also a
test for this delegation.

I've also tidied up the README documentation for custom processors,
hopefully to be more clear than before. There was some duplicated
content between the "Post Processing" section and the "Custom Attachment
Processing" section, and those sections were separated in the file. I've
dedicated the "Post Processing" section to Paperclip's built in
thumbnailing processors, and made "Custom Attachment Processing" section
solely about writing & using your own custom processors. This should be
a more understandable progression, as built-in functionality is
discussed first & separately from extending Paperclip.k
@creature creature force-pushed the readbyte-and-readme-tweaks branch from 35e3b18 to 90c80bb Compare August 3, 2016 01:13
@creature
Copy link
Contributor Author

creature commented Aug 3, 2016

@tute I've rebased this onto the master branch, squashed it down to a single commit, and included a slightly-abridged form of my PR comment as the commit message. Let me know if there's any further changes you'd like to see to this! :)

@tute tute merged commit 90c80bb into thoughtbot:master Aug 5, 2016
@tute
Copy link
Contributor

tute commented Aug 5, 2016

Thank you!

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.

3 participants