Catch 'PermissionError' for unreadable directories #1706
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1706 +/- ##
==========================================
- Coverage 94.55% 94.52% -0.04%
==========================================
Files 79 79
Lines 11754 11760 +6
==========================================
+ Hits 11114 11116 +2
- Misses 640 644 +4
Continue to review full report at Codecov.
|
jedi/api/project.py
Outdated
| try: | ||
| if path.joinpath(name).exists(): | ||
| return True | ||
| except PermissionError: |
There was a problem hiding this comment.
Just open for discussion: Shouldn't we use OSError here? I'm pretty sure that we do not care here if we can read it. If it's not readable, it should probably be ignored.
There was a problem hiding this comment.
I just replaced PermissionError by OSError
| if parent.joinpath(filename).is_file(): | ||
| return parent | ||
| except PermissionError: | ||
| continue |
|
I just added some tests |
test/test_inference/test_sys_path.py
Outdated
| ] | ||
| ) | ||
| def test_get_parent_dir_with_file(path, filename, expected): | ||
| assert sys_path._get_parent_dir_with_file(path, filename) == expected |
There was a problem hiding this comment.
Since os.path.basename(__file__), might be part of a directory ancestor, we should not test like this. Most of the time this will not be an issue for people, but if it is it's very confusing.
IMO this function is tested well enough with other means and we can probably just omit this test.
There was a problem hiding this comment.
Sorry, I removed it
Hi. The master branch of Jedi raises a
PermissionErroron iOS. I added two try excepts on code that assumes a directory is readable. That may be useful on other platforms too. That was the error:/private/var/mobile/Library/Mobile Documentsis the directory where all iCloud Drive files are stored, but it's sandboxed so apps can only read their own container which are located in this directory.Sorry, I don't think iOS is a priority here, but other computers with strict permissions could also raise this error so I think it would be better anyway to handle those cases.