Convert Asserts to Errors

[zm711]

Fixes #847

As part of the push to 1.0.0 we really should move assert to an appropriate error. I'm going to try to start moving some files over to errors.

python -O some_script.py

disables all asserts. So we should make these errors. Just in draft for now.

[zm711]

Do we even want that assertion error test? I can change it....-- I think maybe even making something like NeoWriteError and NeoReadError might be more informative to end users in the end similar to what was suggested in the issue.

[h-mayorquin]

For discussion but probably does not matter I still feel that some asserts that are about input checking should probably remain asserts.

That way, the optimized run of python avoids all of those checks when the user is sure that his data makes sense and wants to get a (marginal) speed up by avoiding some structural checks.

Check this on food for though: https://discuss.python.org/t/stop-ignoring-asserts-when-running-in-optimized-mode/13132/1

To be honest, given that almost no one uses the O flag I think this most of the time does not matter. I think providing better assertion errors is a where the most gain could be.

[h-mayorquin]

To add to this point, dropping assertions is the only thing that the -O flag does.

[zm711]

If you are raising AssertionError for testing things which aren’t assertions about your code’s correctness, e.g. for checking data validity, you are doing it wrong. It is never appropriate to make an assertion about external data, e.g. user-supplied data, arguments passed to a public function by the caller, etc.

quote from the article that I found pertinent. This is basically all neo :)

My thought process is (in this order)

1. We need better messaging. Some our asserts are blank asserts that don't help the user at all. This was an excuse to try to provide meaningful feedback (correct error + better message). This did mean I changed some asserts to other errors and could lose user optimization, but the time scales are minuscule for that type of optimization.
2. Professional-looking code. I think rightly or wrong there is an element of wanting your code to smell right and having too many asserts doesn't give the best impression (same reason why people want to hit the 1.0 because it flags that the code is ready even if it hasn't changed for ages).
3. -O: time gain would be tiny to keep assert whereas our code would currently break without asserts. we could more discerning in leaving some asserts, but then that leads to ego coming into which asserts are good asserts and which are bad

Thanks for the debate though. I think this is super valuable for my own schema of writing code :)

[h-mayorquin]

Your reasoning makes sense to me.

One incentive for people to use asserts instead of errors is that it is easier to comply with code coverage metrics because you don't need to really test the if condition.

[zm711]

One incentive for people to use asserts instead of errors is that it is easier to comply with code coverage metrics because you don't need to really test the if condition.

I'm a bit torn on this. Coverage is a metric to make sure you're actually testing your code. But just trying to get to 100% often leads to pointless tests (just checking an error if statement usually just leads to broken tests when someone just changes the ErrorType--so it's more a change detector than actual test). But it's fair we do want to make sure we aren't missing large chunks of code. I can always say that Neo isn't currently doing coverage anymore so it's not a problem if we aren't looking....

[h-mayorquin]

I agree with you. It is a bad incentive. I was mentioning this as another reason why the (bad) practice is prevalent besides the synatx being more convenient and that you don't have to think on the error. A part of the systemic explanation on why this is so prevalent if you wish. I think python got caught up in a bad equilibrum on this one.