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

Use of assert #112

Open
davidwilby opened this issue Apr 26, 2024 · 1 comment
Open

Use of assert #112

davidwilby opened this issue Apr 26, 2024 · 1 comment
Assignees
Labels
maintenance Tasks relating to codebase maintenance, not functionality

Comments

@davidwilby
Copy link
Collaborator

Mainly just keeping this here as a note to myself to potentially address in future.

I noticed that assert is used (understandably) throughout deepsensor for checking values, sizes etc.

As far as I understand, assert in "production" code isn't advised. This is mainly because when run in optimized mode python -0 <script.py> (or via some other means), the internal __debug__ variable is set to False and stops respecting assert statements. (See official docs: https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement)

assert is essentially:

if __debug__:
    if not expression: raise AssertionError

Now, this is potentially a minor case, but if used in a production setting, it's possible that deepsensor could be used with python's optimized mode and fall over.

I'm in two minds about this because assert statements are concise, but potentially hazardous in the long-run.

Instead of assert, errors should be handled explicitly with raise (as far as I understand it).

@davidwilby davidwilby added the maintenance Tasks relating to codebase maintenance, not functionality label Apr 26, 2024
@davidwilby davidwilby self-assigned this Apr 26, 2024
@tom-andersson
Copy link
Collaborator

Thanks for raising @davidwilby, interesting, I wasn't aware of this! I'm also in two minds about the readability sacrifice, but if you feel strongly feel free to submit a PR.

As you suggest though, this probably isn't a top priority until DeepSensor is likely to be used in a production setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Tasks relating to codebase maintenance, not functionality
Projects
None yet
Development

No branches or pull requests

2 participants