-
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
Close file handles of Popen #4
Conversation
Python started complaining that certain files have been left unclosed. To fix this, always make sure to close all file descriptors simply by wrapping the calls to Popen with a context manager as specified in the python documentation See: https://docs.python.org/3/library/subprocess.html#popen-constructor
One note about Python 2 support. I tried testing it against Python 2.7 as it seems like |
Thanks for your patch. Python 2.7 is still supported, don't worry about testing for it, the CI should take care of it. It does mean I'll have to ask for the |
Sure, I can rewrite the code to use |
As support for python 2.7 should be maintained, a different approach has to be used. `Popen` does not implement a context manager (i.e. no __enter__ and __exit__ methods). The alternative is a `try: ... finally: ...` block. Note that I decided to put `Popen` inside this block. Thus, I had to set proc to be `None` as the `Popen` constructor itself could also raise an exception. Whether this approach is actually reasonable is debatable.
Python 2.7 should work now 😄 |
What irony... Now
|
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.
You would also have to put this comment in the _get_cmd_help_text
function. I added the same try: ... finally
block there as well
Thank you for merging this so quickly 👍🏽 |
Published with |
First of all, thanks for this awesome project. We recently added it to our WeasyPrint pipeline and enjoying it so far!
However, python started complaining that certain files/processes have been left unclosed:
To fix this, I closed all file descriptors simply by wrapping the calls to Popen with a context manager as specified in the python documentation.
See: https://docs.python.org/3/library/subprocess.html#popen-constructor