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

Already on GitHub? Sign in to your account

Fixed file descriptors leak in paperclip_loader #16

Merged
merged 1 commit into from Mar 28, 2013

Conversation

Projects
None yet
2 participants
Contributor

arr-ee commented Mar 28, 2013

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 added a commit that referenced this pull request Mar 28, 2013

Merge pull request #16 from arr-ee/master
Fixed file descriptors leak in paperclip_loader

@autotelik autotelik merged commit d6716b9 into autotelik:master Mar 28, 2013

Owner

autotelik commented Mar 28, 2013

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

Contributor

arr-ee commented Mar 28, 2013

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.

Owner

autotelik commented Mar 28, 2013

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.

autotelik added a commit that referenced this pull request Aug 24, 2014

Merge pull request #16 from arr-ee/master
Fixed file descriptors leak in paperclip_loader

Former-commit-id: d6716b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment