foroozandehgroup / esrpoise

Parameter Optimisation by Iterative Spectral Evaluation: an Xepr-compatible ESR package
GNU General Public License v3.0
1 stars 0 forks source link

don't catch all Exceptions in `Xepr_link.py` #7

Closed yongrenjie closed 3 years ago

yongrenjie commented 3 years ago

there are a bunch of stuff in Xepr_link.py which basically do

try:
    run_some_stuff()
except Exception:
    print("some_error_happened")
    sys.exit(exit_code)

in general it's not good practice to capture all types of exceptions as that could lead to a bug. do we know what exceptions XeprAPI might throw (eg does it throw a specific class of exceptions like XeprError)? if so then we should either exclusively catch that, OR

even better, just don't catch it. because we're catching it only to exit again - there's not much point in doing this, because when the exception is thrown the code will exit anyway. on top of that it is more useful to the user because (1) it will print the error traceback which is generally pretty useful (2) the user can catch the error themselves if they want to do something, whereas sys.exit basically just kills the entire thing and doesn't give the user a choice.

if we want to show a helpful error message to the user on top of that, then this is the 'correct' form:

try:
    run_some_stuff()
except XeprError as e:    # or if there isn't a specific error type, then just Exception
    print("some error happened! here's what you can do about it: ...")
    raise
JB-V commented 3 years ago

This structure may have originally in place because it was written in Python 2 or maybe it was necessary to get an error message back in matlab? There isn't much error handling in Xepr. I guess we should get rid of it alltogether

yongrenjie commented 3 years ago

Looking back at this, yes, I suspect the original intent was to send some information which could be interpreted by Matlab.

In our case, a more "idiomatic" solution would be to raise ...Error("error_message"); and I think the most appropriate builtin class is RuntimeError, which is basically a catch-all for "something went wrong while running this code which we couldn't plan for".