Use a block with file.new to auto-close file ref #191
Conversation
| scan = ::File.new(zip_file, 'rb') | ||
| data.add_part(scan.read, 'application/zip', 'binary', | ||
| "form-data; name=\"scan\"; filename=\"#{zip_file}\"") | ||
| ::File.new(zip_file, 'rb') do |scan| |
There was a problem hiding this comment.
@gschneider-r7 Why do you need :: for ::File.new instead of directly calling File.new?
There was a problem hiding this comment.
Beats me, it was already there. I assumed there were good reasons at some point.
There was a problem hiding this comment.
I don't think so in this case, to my knowledge usually we use :: to call Module::Class to know which class we are calling. Could you please remove the ::?
There was a problem hiding this comment.
Just remembered File.new doesn't accept a block, we need to use File.open instead.
(irb):2: warning: File::new() does not take block; use File::open() instead
There was a problem hiding this comment.
I don't think so in this case, to my knowledge usually we use :: to call Module::Class to know which class we are calling. Could you please remove the ::?
Using ::File over File I assume at one point (if not still the case) we had a conflicting constant in the Nexpose module namespace. Using ::File explicitly gets the top level constant (out of the Nexpose namespace) and leaving it alone doesn't affect this fix (while changing it could introduce a bug theoretically).
There was a problem hiding this comment.
Ah I thought File.new and File.open were actually the same method (aliases). I'll change it.
There was a problem hiding this comment.
@rkhalil-r7 @gschneider-r7 yep, definitely have at least one today see lib/nexpose/asset.rb.
|
@gschneider-r7 Let's change to using |
Use a block with file.new to auto-close file ref
Fixes #190
I haven't tested this yet, but the behavior should be exactly the same except that the file will auto-close when the block ends. The export scan method also does this, although using the one-liner bracket syntax.