Skip to content
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

Don't use bare except when importing #667

Merged
merged 1 commit into from Feb 22, 2024

Conversation

Singletoned
Copy link
Contributor

Using a bare except statement when importing hides other errors, which then get lost when the next import fails.

Using a bare except statement when importing hides other errors, which
then get lost when the next import fails.
@rayluo
Copy link
Collaborator

rayluo commented Feb 20, 2024

Thanks for your suggestion. We could have been written in the way that you proposed. Out of curiosity, under what scenario did you run into that import raising non-ImportError?

Besides, the current implementation dates back to Python 2.7 era, and we will probably remove the entire try...except... in the foreseeable future, making this pattern irrelevant.

@Singletoned
Copy link
Contributor Author

I don't actually know what caused the non-ImportError. It was a one off occurrence in our logs, and the true error was disguised by the bare except, hence the PR. I'd be grateful if this could be fixed, so that if it happens again, we'll have the correct traceback in our logs.

I strongly recommend never using a bare try...except... at all, and in fact most linters will automatically pick this up and mark it as an error.

Note that you are using a better pattern just a few lines below the change in this PR.

A better pattern overall is to not use try at all, and to explicitly test for a version of Python and modify the behaviour according to that (which is what you are doing in other places in the code).

Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for the report.

If you would also like to add more commits into this PR to change other bare except, we will happily take it. Or we can also accept this PR as-is.

@Singletoned
Copy link
Contributor Author

Thanks! I'll do the others in other PRs, as and when I come across them

@rayluo rayluo merged commit 9a866ca into AzureAD:dev Feb 22, 2024
8 of 9 checks passed
@rayluo rayluo mentioned this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants