search

autotelik / datashift

Full Excel/CSV Import/Export facilities for Rails
http://www.autotelik.co.uk
Other
136 stars 75 forks source link

Fixed file descriptors leak in paperclip_loader #16

Closed arr-ee closed 11 years ago

arr-ee commented 11 years ago

Fixes autotelik/datashift_spree#10

Looks like paperclip expects that file would be closed somewhere else. Actually I got lost somewhere in it so this one may be a little hackish.

Also sorry for the mess with whitespaces, please tell if that's a problem — I'll fix that.

autotelik commented 11 years ago

Hi Max .. great stuff

I'll merge this in for now, but really you've highlighted that the get_file method is not too great, or at least badly named ! .. not clear that handle needs closing .. even to me and I wrote the damn thing !

Will review after me Easter hols

many thanks for contributing tom

arr-ee commented 11 years ago

Hey Tom, thanks for the fast response!

Actually naming is okay, but maybe it'd be better to mimic File.open w/ block approach (you know, we work with file in the block and then it gets closed automatically).

Or maybe it'd be even better to pass File object into the loader — less object allocations, less everything + it already ensures it's being closed afterwards.

Or even mix this approaches. I'll try to play with that during weekend.

autotelik commented 11 years ago

Hi Max

yeah agreed .. I was just thinking same - mimic the nice/familiar blocks style of File.open

cheers tom

From: Max Barnash notifications@github.com To: autotelik/datashift datashift@noreply.github.com Cc: tom statter github@autotelik.co.uk Sent: Thursday, 28 March 2013, 20:06 Subject: Re: [datashift] Fixed file descriptors leak in paperclip_loader (#16)

Well actually it's okay, but maybe it'd be better to mimic File.open w/ block approach (you know, we work with file in the block and then it gets closed automatically). Or maybe it'd be even better to pass File object into the loader — less object allocations, less everything + it already ensures it's being closed afterwards. Or even mix this approaches. I'll try to play with that during weekend. — Reply to this email directly or view it on GitHub.