gh-125522: Fix bare except in test_uuid
#129018
Conversation
tomasr8
added
skip news
needs backport to 3.12
Co-authored-by: Thomas Grainger <[email protected]>
| @@ -21,7 +21,7 @@ def importable(name): | |||
| try: | |||
| __import__(name) | |||
| return True | |||
| except: | |||
| except ModuleNotFoundError: | |||
There was a problem hiding this comment.
If you want to be maximally pedantic you can go even more specific:
| 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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Yeah I'm cool with it either way
There was a problem hiding this comment.
Alright, I'll keep it as is for now and let's see what other reviewers think :)
| @@ -21,7 +21,7 @@ def importable(name): | |||
| try: | |||
| __import__(name) | |||
| return True | |||
| except: | |||
| except ModuleNotFoundError: | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Not 100% sure, but I think anything else would be unexpected here and would warrant looking into
|
Thanks @tomasr8 for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
(cherry picked from commit a1a4e9f) Co-authored-by: Tomas R. <[email protected]>
|
GH-129455 is a backport of this pull request to the 3.13 branch. |
(cherry picked from commit a1a4e9f) Co-authored-by: Tomas R. <[email protected]>
|
GH-129456 is a backport of this pull request to the 3.12 branch. |
The intent of the helper function is only to check whether a module can be imported, so catching all exceptions could hide other issues.
(I'm adding the backport labels seeing as the other PRs were also backported)