Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-125522: Fix bare except intest_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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Thomas Grainger <tagrain@gmail.com>
| __import__(name) | ||
| returnTrue | ||
| except: | ||
| exceptModuleNotFoundError: |
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.
If you want to be maximally pedantic you can go even more specific:
| exceptModuleNotFoundError: | |
| exceptModuleNotFoundErrorase: | |
| ife.name==name: | |
| returnFalse | |
| 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.
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 ate.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.
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
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.
Alright, I'll keep it as is for now and let's see what other reviewers think :)
| __import__(name) | ||
| returnTrue | ||
| except: | ||
| exceptModuleNotFoundError: |
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.
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.
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
a1a4e9f intopython:mainUh oh!
There was an error while loading.Please reload this page.
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 commita1a4e9f)Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
GH-129455 is a backport of this pull request to the3.13 branch. |
(cherry picked from commita1a4e9f)Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
GH-129456 is a backport of this pull request to the3.12 branch. |
Uh oh!
There was an error while loading.Please reload this page.
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)