check for writable dirs, not just existence, in utils.path #670
Conversation
replaces various calls to os.path.isdir and os.path.exists with _writable_dir. closes ipythongh-669
| @@ -166,7 +170,7 @@ def get_home_dir(): | |||
| raised for all other OSes. | |||
| """ | |||
|
|
|||
| isdir = os.path.isdir | |||
| isdir = _writable_dir | |||
There was a problem hiding this comment.
I'd use the name is_valid_dir to avoid getting confused later down in the code by thinking that isdir is just an alias to os.path.isdir when in reality it's a wrapper with additional specific functionality.
|
Could you add a test for this? I think simply creating a directory without write permissions and then trying to use it as IPYTHON_DIR should be very easy to do. But it will help ensure that our error checks remain valid accross OSes. That kind of thing sometimes has funny platform variability, so I'd feel better with an actual test that buttresses this code. Thanks! |
add test for ipythongh-669, and ignore nonexistent toy paths in other path tests.
|
test added, and tempdir behavior implemented. The tempdir behavior meant I had to tweak many other tests, since they use toy non-existent directories. Due to the test changes, wait for a Windows test to merge. |
|
Sorry Min, what do you mean by "wait for a Windows test to merge"? That you are going to do some Win testing yourself and don't want the merge to be done before that? Or that you need some help with Windows-specific testing? |
|
Yes, I just mean wait until I run the test suite on a Windows machine. Since I had to make some changes to existing tests, it's possible that one or more of the Windows-only tests will also require a change. |
It doesn't seem possible to create a dir that the owner can't write to on Windows, so the test doesn't run properly. If the test can be updated to reliably create/find a user read-only directory (probably via NTFS ACLs and win32security), then it can be unskipped.
|
I've had to skip the new test on Windows, because I can't figure out how to create a read-only dir on Windows. Otherwise, it still passes. |
|
Looks good, thanks! Merging now. |
replaces various calls to
os.path.isdirandos.path.existswith_writable_dir, which checks existence and write-permission.should close #669