Skip to content

gh-125522: Fix bare except in test_uuid #129018

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 2 commits into from
Jan 29, 2025
Merged

gh-125522: Fix bare except in test_uuid #129018

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7529447
Fix bare except in test_uuid
tomasr8 Jan 19, 2025
5f5c0db
Use `ModuleNotFoundError`
tomasr8 Jan 19, 2025
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/test/test_uuid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def importable(name):
try:
__import__(name)
return True
except:
except ModuleNotFoundError:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to be maximally pedantic you can go even more specific:

Suggested change
except ModuleNotFoundError:
except ModuleNotFoundError as e:
if e.name == name:
return False
raise

Eg if a module is successfully found but then imports another module which isn't found, this will raise but the code in the PR will not

Copy link
Member Author

Choose a reason for hiding this comment

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

There's only one other place in the repo that looks at e.name so this seems to be a pretty uncommon pattern and maybe not worth for test code? Though I don't have a strong preference either way :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm cool with it either way

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I'll keep it as is for now and let's see what other reviewers think :)

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this is the only exception type that can come from __import__(name)?

I guess since it's a test it's fine to assume (if the tests pass).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% sure, but I think anything else would be unexpected here and would warrant looking into

return False


Expand Down
Loading