Raise exception rather than returning 0/calling sys.exit

[wtbarnes]

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.

[MJWeberg]

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.

[wtbarnes]

I was just really bitten by this issue because of this bit of code,

eispac/eispac/core/export_fits.py

Lines 68 to 70 in 4228525

	if not output_dir.is_dir():
	print("Error: sav_dir is invalid or missing!", file=sys.stderr)
	return None

Because an error is not raised, it is really easy to fit a lot of spectra without realizing that nothing is being saved since execution is not interrupted. One could of course argue that I should have (1) paid attention to what was being printed to STDERR and (2) noticed that no files were actually being saved. That being said, I think not throwing an exception results in a frustrating user experience, particularly when running in a "batch" processing mode.

In particular, I'd argue printing errors and not explicitly throwing an exception goes against the "Errors should never pass silently." bit of the Zen of Python.

tl;dr I really think we should consider moving away from this pattern of print statements/retrurn None/sys.exit and make use of logging and/or exceptions wherever appropriate (see also #26). This need not be done all at once and can be done piecemeal, but I'd like to get an idea as to whether there is consensus on this approach before going down this road.

[MJWeberg]

Sorry this caused some issues and headache!

You make some very good points. The original intent was to be "batch processing friendly" such that one could set the code running on dozens (or hundreds) of files and not have it crash halfway through due to a single, bad data file. However, it seems that we went a bit overboard with this. I agree that the code snippet above really should raise an error, since it has nothing to do with a specific file and causes minor filepath typos to generate unexpected results. I will go through the code and try to fix other obvious cases like this.

You are right that we should also reevaluate the use of this design pattern in general. It is probably better to have user-tutorials, and official, batch processing functions (like the eis_fit_files script) rather than trying to have built-in compatibility with non-standard behavior. We want to be as user-friendly as possible, but not at the expense of clarity and good coding habits.

Thanks for the feedback and following up on this!