Skip to content

Make _is_writable_dir more flexible to obscure failure modes #4165

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 3, 2015
Merged

Make _is_writable_dir more flexible to obscure failure modes #4165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/matplotlib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ def _is_writable_dir(p):
t.write(b'1')
finally:
t.close()
except OSError:
except:
Copy link
Member

Choose a reason for hiding this comment

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

don't forget a "# noqa" flag here. I would also think it would be better to do catch Exception as a bare except catches everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would it catch besides a subclass of Exception?

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand there is still code out there that raises strange things (like strings), catching everything here seems like the correct thing to do 'Can we write to this dir yes or no?' I don't think we really care why we can't write there.

Copy link
Member

Choose a reason for hiding this comment

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

@embray, there is a subtle difference between a bare exception and catching Exception. A bare exception is basically equivalent to "except BaseException:", which isn't the same thing as "except Exception:". But, @tacaswell has a good point, we want to be able to catch any sort of failure, for whatever reason and just move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't there was a separate BaseException under Exception, but all the more reason not to explicitly catch just Exception.

I don't see how could could "raise strings"--that would result in a TypeError.

Copy link
Member

Choose a reason for hiding this comment

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

I thought the restriction that you could only raise Exception objects was new-ish and we support back to 2.6 (but I couldn't figure out quickly when that got changed).

Copy link
Member

Choose a reason for hiding this comment

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

I thought the restriction that you could only raise Exception objects was new-ish and we support back to 2.6 (but I couldn't figure out quickly when that got changed).

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we are clear on that. Just tried on py2.6.

[centos6] [broot@rd22 gust_checks]$ /usr/bin/python
Python 2.6.6 (r266:84292, Jan 22 2014, 09:42:36)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> raise "SomeString"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: exceptions must be old-style classes or derived from
BaseException, not str
>>>

On Mon, Mar 2, 2015 at 4:43 PM, Thomas A Caswell notifications@github.com
wrote:

In lib/matplotlib/init.py
#4165 (comment):

@@ -225,7 +225,7 @@ def _is_writable_dir(p):
t.write(b'1')
finally:
t.close()

  • except OSError:
  • except:

I thought the restriction that you could only raise Exception objects was
new-ish and we support back to 2.6 (but I couldn't figure out quickly when
that got changed).

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4165/files#r25641395.

return False

return True
Expand Down