-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
gh-131647: fix 'sys.path_hooks is empty' warning in test_permission_e… #131648
Conversation
@@ -158,11 +158,15 @@ def test_deleted_cwd(self): | |||
def test_permission_error_cwd(self): | |||
# gh-115911: Test that an unreadable CWD does not break imports, in | |||
# particular during early stages of interpreter startup. | |||
|
|||
def noop_hook(*args): |
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.
noop name is misleading, since the hook actually does something, raise ImportError :-) I suggest to rename the function to something like failing_path_hook
.
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.
I'm not familiar with path_hooks but a noop hook is one that causes the processing to continue checking other hooks and return None if there's no others
This hook doesn't fail, eg the error is immediately caught and has no change in behavior over having no hooks other than not warning
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.
I'm happy to rename it, but with "failing_path_hook" it would appear that the test was about the failing hook rather than the permission error because the new name attracts too much attention
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.
Well, the name of the function doesn't matter much. I merged your PR ;-)
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.
LGTM. I just have a minor suggestion.
Merged, thanks for the fix. |
…rror_cwd