eispac icon indicating copy to clipboard operation
eispac copied to clipboard

Raise exception rather than returning 0/calling `sys.exit`

Open wtbarnes opened this issue 2 years ago • 1 comments

There are several places in the codebase where either an error is printed to the screen using print, 0.0 is returned, or sys.exit() is called. Instead, in all of these cases, an exception should be raised instead with an appropriate message. This will print the entire stack trace to STDERR and is, in general, the Pythonic way of telling the user something has gone wrong.

If there is a situation where you want to tell the user something may have gone wrong, but don't want everything to necessarily blow up in their face, raising a warning is probably the best option.

In Python, you can even define your own custom exceptions (as an example) to raise even more informative errors.

wtbarnes avatar Sep 23 '21 17:09 wtbarnes

This is a very good point! We actually were raising exceptions at first; however, after some debate, we decided to just print an error message and return 0 or None instead (at least for simple IO errors or certain, specific issues). One of the potential use cases of eispac involves batch processing multiple files and spectral windows all at once. In such cases, it is preferable that the program gracefully skips any broken files or missing spectral windows, rather than bringing the entire operation to grinding halt part way through. While users can and should use try-except blocks, there is no guarantee that they would see the error messages and, most significantly, some of our target users are long-term IDL programmers who may not have learned the correct usage of Python’s try-except blocks yet.

We are, however, not completely set on this design pattern. We will probably revisit the topic once we have a proper logging system in place and, perhaps, add a tutorial to the user’s guide demonstrating best practices for batch processing files. The few places where sys.exit() are still used are either used for debugging (and therefore are never encountered by users during normal operations) or are part of the gui applications where printing the entire stack trace is unhelpful to users. Nevertheless, we should reevaluate those instances and remove or replace as needed.

MJWeberg avatar Oct 07 '21 16:10 MJWeberg