ENH: switch to 'with' statement for open/close file in lib.npyio._savez - #6545

I don't think your context handler is exception safe, so either needs a fix or this should still be in a try..except clause.

OK, exception handler will reraise.

As the exception handler was only here to remove/close files in the original version, no need to add try/except mechanism to the with statement.
This this exactly the case given in the "Motivation and summary" section of pep-0343 .

Maybe something simpler?

@contextmanager
def TempFileName(suffix='', prefix='tmp', dir=None):
    fid, name = mkstemp(suffix, prefix, dir)
    os.close(fid)
    yield name
    os.remove(name)

This is a common pattern in a lot of tests, e.g, in test_io.py

Nb that that's not the right way to write a context manager -- if the with
block raises an exception then the yield will raise that exception and
os.remove won't be called, you still need a try/finally.
On Nov 5, 2015 2:03 PM, "Charles Harris" [email protected] wrote:

In numpy/lib/npyio.py
#6545 (comment):

@@ -599,13 +611,41 @@ def savez_compressed(file, _args, *_kwds):
"""
_savez(file, args, kwds, True)

[email protected]

Maybe something simpler?

@contextmanager
def TempFileName(suffix='', prefix='tmp', dir=None):
fid, name = mkstemp(suffix, prefix, dir)
os.close(fid)
yield name
os.remove(name)

This is a common pattern in a lot of tests, e.g, in test_io.py

—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/6545/files#r44076176.

OK, same thing rewritten as an official context manager ;)

@njsmith I believe that for real exception handlers __exit__ is called, then the exception is reraised.

we also often just use a temporary directory for this situation, a context manager for that is pretty simple (and also already exists in python3.x) we really should have it in our test utils

oh we already have one but its not correct ...

@juliantaylor See also the discussion at #6660.

Isn't this also a reasonable place to use contextlib.closing, instead of creating your own class? This seems like exactly the situation that function / decorator exists...

(PS sorry for the double comment, the last one was in the wrong place)

@wackywendell not here but for the zipfile, yes

Right, sorry, I don't know why I'm having so much trouble putting my comments on the right line.

hm where is the os.close?

The os.close is automatically performed by the context manager of NamedTemporaryFile at the exit of the with statement (here before line 633).

The whole pain came from the fact that NamedTemporaryFile is a factory function not a class, so you can not subclass it.

What happens when an error is raised from yield?

If you mean by the caller of DeleteOnContextExitNamedTemporaryFile before the return of "yield fid" then it calls the exit method of fid. That is the purpose of using the context manager NamedTemporaryFile. See
https://www.python.org/dev/peps/pep-0343

The exit method will close but not delete the file.
After that our context manager takes the relay and deletes the file.

Exit method of NamedTemporaryFile, which closes the file but doesn't delete it. The __exit__ method will then reraise the error and you will not catch it.

OK, I've described the good way of doing it by rewritting class _TemporaryFileWrapper from tempfile.py on my last comment on #6866

I think you'd want ExitStack here to implement this properly