mikeshultz / ledger-eth-lib

Library to interface with Ethereum app on Ledger hardware wallets
https://ledgereth.readthedocs.io/
MIT License
19 stars 9 forks source link

assert statements in code #17

Closed antazoey closed 2 years ago

antazoey commented 2 years ago

I have noticed asserts statements around, like here: https://github.com/mikeshultz/ledger-eth-lib/blob/master/ledgereth/objects.py#L36

I want to caution that when running python with the -O flag, these asserts get stripped out of the code. So if you are expecting an error, it wouldn't happen in that case.

It is less clean, but I find it is just better to raise AssertionError or a custom error for this reason.

Here is some more info: https://bandit.readthedocs.io/en/latest/plugins/b101_assert_used.html

mikeshultz commented 2 years ago

hm, I might argue about some of the asserts in any other context because one who runs with python -O is asking for it. However, the example you provided is just a way to trigger the raising of the ValueError and doesn't make a whole lot of sense as it is. I probably just chose that pattern because it was prettier than if/and/and/and.

antazoey commented 2 years ago

hm, I might argue about some of the asserts in any other context because one who runs with python -O is asking for it.

That is a fair point. I think it just depends on the context of the assert. Like I still use them to make mypy happy but mypy to me is just a debug-related tool for types, so I definitely do want those stripped out. But normal exceptions I expect to be raised at runtime should be there in an optimized mode imo

mikeshultz commented 2 years ago

Like I still use them to make mypy happy but mypy to me is just a debug-related tool for types, so I definitely do want those stripped out.

oh nice, might start using that, thanks for the tip.