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

fix for temporary files without the original_filename attribute. #331

Closed
wants to merge 2 commits into from
Closed

Conversation

MaG21
Copy link

@MaG21 MaG21 commented Mar 18, 2014

This PR fix a bug in the TempObject, now one can use temporary files in conjunction with dragonfly without losing the original name of the temporary file, even if the original_filename attribute it's not defined.

how to reproduce the actual bug
ie:

tmp_file = Tempfile.new(['hola', '.txt'], :encoding => 'ascii-8bit')
tmp_file.write(content)
img_uid = app.store(tmp_file)
puts img_uid

will output something similar to this:
2014/03/18/11/47/41/506/file

With this PR output should be something similar to:
...path.../hola2843-84--0.txt

@markevans
Copy link
Owner

thanks. please could you put tests in?

@MaG21 MaG21 closed this Mar 18, 2014
@MaG21 MaG21 reopened this Mar 18, 2014
@MaG21
Copy link
Author

MaG21 commented Mar 18, 2014

mm ... ok

I see, there is a test case against this PR here: temp_object_rspec:384

code:

it "should not set the name if the initial object doesn't respond to 'original filename'" do
  Dragonfly::TempObject.new(@obj).original_filename.should be_nil
end

@markevans Why?

@markevans
Copy link
Owner

actually why do you want the original filename of the tempfile? is it for the file extension? What's the actual use case? (this is internal code which isn't usually called by the user)
I don't think the whole file name is desired as it's normally a long ugly random string

@markevans
Copy link
Owner

closing as the correct way is to set the name attribute anyway

img_uid = app.store(tmp_file, 'name' => 'hola.txt')

@markevans markevans closed this Apr 29, 2015
@MaG21
Copy link
Author

MaG21 commented Apr 30, 2015

First at all, I'm sorry, I don't intend to extend this any further, mostly because this is an old thread. However, I do want to say one more thing before closing this thread for good.
...

Most of the time, we use temporary files because we don't care about the location it have been saved and/or the file name, specially the name.

img_uid = app.store(tmp_file, 'name' => File.basename(tmp_file.path))

The suggestion above will go against the DRY principle (Rails's motto), because the programmer repeats himself. This information (the name) already lies in the object.

Also it will mark an inconsistency with the actual API, because one doesn't necessary have to do this for other objects, ei. an instance of File but for Tempfile one must.

@waiting-for-dev
Copy link

I think my issue is related with this old post.

I'm using memory datastore in my test environment. Now, in my code I'm using a gem which takes an image path argument. I'm providing image.path, but then it raises an error because it returns an string without extension (that gem is checking against the extension). If I change my datastore to file everything works fine.

By the way, the gem I'm using is axlsx.

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.

3 participants