-
Notifications
You must be signed in to change notification settings - Fork 10
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 #313 remove handle #315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, works well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The smart thing about the with
statement and the use of tempfile.NamedTemporaryFile()
is that the temporary file is deleted upon exiting the with
block.
Could you possibly add this deletion manually here instead to ensure we don't clog up the temporary location?
According to the documentation we cannot use tempfile.NamedTemporaryFile() in this way on windows: I agree with Casper about deleting the file. This shows us that we probably should have some windows environment tests in the CI as well... |
Would it help to add the argument |
According to the reference text copied by Francesca, I don't think it will help to add 'w'. So I will try to add the file delete. Otherwise, yes it should be mandatory to test it on Windows, especially for a tool aimed at a larger audience. |
Nope, I tested. It would maybe work with the option ´delete=False´, but then we have to delete the file afterwards as well. |
Description:
Type of change:
Checklist:
This checklist can be used as a help for the reviewer.
Comments:
the following code creates a problem on Windows:
It is related to an issue with tempfile library. The problem comes from reopening the temporary file. the first line create and open the file. so the modification simply create a file name located in the temporary folder and use that path for the successive operations.