IO_Adapter methods and rewind before saving to S3 #838

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

fjg commented Apr 22, 2012

I recently updated a project and needed to add missing methods on the new FileAdapter (readbyte, readchar).

Also I was using paperclip-aws and migrated to paperclip that switched to aws-sdk. But having a post_processors that extract informations from attachment make them unreadable on S3 cause of the missing rewind before write to S3 (as paperclip-aws does).

Contributor

sikachu commented Apr 23, 2012

I understand that this is missing. However, do you mind implementing the test for them as well?

Thank you.

fjg commented Apr 23, 2012

sure ;)

Member

jyurek commented Apr 23, 2012

May I ask what you need these methods for? I don't really want to proxy a bunch of methods to Tempfile, and if there's a way you can accomplish your task without them, that would be preferred.

@fjg fjg closed this Apr 24, 2012

@fjg fjg reopened this Apr 24, 2012

fjg commented Apr 24, 2012

I am using exifr to extract some data about the picture in post_process and it needs readbyte/readchar.

I was wondering why rewind had been removed for s3 in the latest versions.

Member

jyurek commented Apr 24, 2012

@fjg, I assume you have the files locally, or you wouldn't be looking to add these methods to the FileAdapter. Is it possible for you to hand those files to exifr manually, without the Adapter intervening?

The rewind call can be merged in, though I would like to see a test for it that confirms the functionality. @fjg, can you resubmit this pull request preferably with a test, but even if not, can you submit it with just the rewind commit and not the readchar commit? I'll close this one for now.

Thanks.

@jyurek jyurek closed this Apr 24, 2012

fjg commented Apr 24, 2012

@jyurek Actually using the attachment during a post_process is easier through the attachment.queue_for_write[:original] method. The file is already open and readable.
I would not like to open it again and read it through File.open/read. Does it make no sense?

OK for the test and pull-request again

fjg commented May 2, 2012

@jyurek @sikachu the rewind was already there. The merge introducing the IO_Adapter removed it (89c8d11). Here is the previous commit with a test that you can merge again: 7cb7384

fjg commented May 2, 2012

@jyurek will you add the readchar/readbyte methods? Do you need a dedicated pull-request?

sikachu added a commit that referenced this pull request Jun 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment