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

Remove empty except clauses #1696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Julian-O
Copy link
Contributor

Empty except clauses are code smell. At the very least, they can unintentionally catch KeyboardInterrupt and SystemExit exceptions. At worse, they can hide bugs.

Looking through Buildozer code, there are four empty except clauses in android.py.

One protects a call to set_win32_java_home() (on Windows machines), Given win32 isn't yet supported, this is moot, but there doesn't seem to be a justification for ignoring exceptions in this scenario - if setting the home fails, Buildozer should fail. Deleted try block.

One protects a parse of the app version number. The called code already catches the most obvious exception (a badly-formatted version number). There don't seem to be any other expected exceptions, so there is not reason to swallow this exception. Deleted try block.

One is harder to understand. After calling execute_built_package(), a try block attempts to call it again, surrounded by calls to run a pre-build and post-build hook on buildozer. However, the hook is never defined anywhere. This always dies with an AttributeError which is swallowed and ignore by the empty exception block, without the duplicate call to build. I am guessing it is a carry-over from an attempt to make Buildozer support hooks. This is an example of why not to use empty Exception blocks! Removed the try block and the protected code.

The fourth example protects an attempt to decode Unicode. content is read from a IO object opened in 'r' mode, so it will always be a str and will never have a decode() attribute, so this will always fail. Removed the code in the try block and just kept the except: part.

[Empty except clauses are code smell](https://stackoverflow.com/questions/14797375/should-i-always-specify-an-exception-type-in-except-statements/). At the very least, they can unintentionally catch KeyboardInterrupt and SystemExit exceptions. At worse, they can hide bugs.

Looking through Buildozer code, there are four empty except clauses in `android.py`.

One protects a call to set_win32_java_home() (on Windows machines), Given win32 isn't yet supported, this is moot, but there doesn't seem to be a justification for ignoring exceptions in this scenario - if setting the home fails, Buildozer should fail. Deleted try block.

One protects a parse of the app version number. The called code already catches the most obvious exception (a badly-formatted version number). There don't seem to be any other expected exceptions, so there is not reason to swallow this exception. Deleted try block.

One is harder to understand. After calling `execute_built_package()`, a try block attempts to call it **again**, surrounded by calls to run a pre-build and post-build hook on buildozer. However, the hook is never defined anywhere. This always dies with an `AttributeError` which is swallowed and ignore by the empty exception block, without the duplicate call to build. I am guessing it is a carry-over from an attempt to make Buildozer support hooks. This is an example of why not to use empty Exception blocks! Removed the try block and the protected code.

The fourth example protects an attempt to decode Unicode. `content` is read from a IO object opened in 'r' mode, so it will always be a `str` and will never have a `decode()` attribute, so this will always fail. Removed the code in the try block and just kept the except: part.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant