Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for fog streaming uploads #467

Conversation

chrisdurtschi
Copy link
Contributor

Fog supports streaming uploads if passed a File object.
Added SanitizedFile#to_file method that returns a File object.
This works for file system based SanitizedFile (File, Tempfile, Hash, etc)
StringIO must still be read into memory before being passed to fog.

Fog supports streaming uploads if passed a File object.
Added SanitizedFile#to_file method that returns a File object.
This works for file system based SanitizedFile (File, Tempfile, Hash, etc)
StringIO must still be read into memory before being passed to fog.
@trevorturk
Copy link
Contributor

@geemus would you mind reviewing this?

# [File] a File object representing the SanitizedFile
#
def to_file
File.open(@file) if exists?
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should perhaps use is_path? for consistency with read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that exists? will work better here. is_path? will return false for a SanitizedFile created from either a File or Tempfile object, which means that the entire contents will be read into memory before being passed to fog.

This code will needlessly create a new File object if @file is already a File. Maybe I should change the first line to this:

return @file if @file.is_a?(File)

@geemus
Copy link
Contributor

geemus commented Sep 20, 2011

Other than that one thing it looks good (basically the main difference is passing a file handle, ie File.open(path) instead of string and it certainly does this.).

Shouldn't rely on #exists? here, since #to_file implementation might
change.
@geemus
Copy link
Contributor

geemus commented Sep 21, 2011

Seems reasonable I think.

@trevorturk
Copy link
Contributor

Thanks for looking @geemus.

@chrisdurtschi I'm getting lots of test failures. Did you run the full test suite with your changes?

     Failure/Error: @fog_file = @storage.store!(@file)
     TypeError:
       can't convert Tempfile into String

Please re-open this pull request or open a new pull request if/when you have time to fix things.

@trevorturk trevorturk closed this Sep 22, 2011
@chrisdurtschi
Copy link
Contributor Author

Sorry, I only tested in 1.9.2. I'm getting these same failures in 1.8.7. I'll look into it and send a new pull request.

@trevorturk
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants