Skip to content
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

Force closing PIL image files #972

Merged
merged 2 commits into from Jul 18, 2012
Merged

Force closing PIL image files #972

merged 2 commits into from Jul 18, 2012

Conversation

cgohlke
Copy link
Contributor

@cgohlke cgohlke commented Jun 29, 2012

This fixes some ResourceWarning: unclosed file warnings under Python 3.2

@@ -1185,12 +1185,20 @@ def imread(fname, format=None):
can be used with :func:`~matplotlib.pyplot.imshow`.
"""

def pilread():
def pilread(fname):
"""try to load the image with PIL or return None"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you have extended the capability of this function (file handles are now supported), can you improve the doctoring a little. Perhaps the variable name fname is no longer appropriate - maybe something like img_file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the capability of the function is the same as before. It just works around another bug in PIL, which does not seem to close all the files it opens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. but the variable naming statement still holds (fname could be a file handle or a filename).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it is a good idea to rename the variable. fname is used as an argument in the outer function. Renaming it there would break the API. Renaming it in pilread only would be confusing I think.

@pelson
Copy link
Member

pelson commented Jun 30, 2012

+1

@mdboom
Copy link
Member

mdboom commented Jul 1, 2012

Looks good.

@WeatherGod
Copy link
Member

what is holding up this PR?

@pelson
Copy link
Member

pelson commented Jul 18, 2012

Nothing.

pelson added a commit that referenced this pull request Jul 18, 2012
Force closing PIL image files
@pelson pelson merged commit 7a6897e into matplotlib:master Jul 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants