AMBER: inconsistent use of ```raise Exception``` / ```logger.exception```

[DrDomenicoMarson]

Going through the AMBER parser I noticed that there is inconsistent use of raise Exception (also a little bit broad as an exception), and logger.exception.

I don't know if there is any specific reasoning behind that, but at a first glance, I think it should be advisable to be consistent.

Some possible scenarios:

a) we leave it like that "if it ain't broke, don't fix it" style b) we unversaly use logger.exception through the code c) we still use raise some time, but we use more precise Exceptions, maybe creating some more precise Exception-subclasses inside util.py

[orbeckst]

logger.exception() is used inside an exception handler and the normal pattern should be

try:
     do_stuff(filename)
except SpecificError:
    logger.exception("Bad stuff happened in %s, beware!", filename)
    raise

In this way, the original SpecificError is raised but fully logged (logger.exception() is a special case of logger.error() that includes the full stack trace to aid debugging).

Generally, I like "Fail early and often" (and leave a good error message when you do) so I'm all for making the parsers more consistent.

I have no problem with the broad exception if it gets raised again (i.e., in the example above, a catch-all except would be fine with me). But I do not like try/except: pass as this will just make debugging impossible.

[DrDomenicoMarson]

I completely agree that try/except: pass is bad practice, I don't like (if possible) also the too-broad exceptions.

I think this issue could be tackled after the two easier #225 and #226 are done, as more code will be edited I think! So if I understand correctly, it's better to uniformly use logger.exception instead of Exception, and the more the exception is focused, the better.

If it will be needed, what's your opinion on creating one or more subclasses of "Exception" inside util.py?

[orbeckst]

If it will be needed, what's your opinion on creating one or more subclasses of "Exception" inside util.py?

If there are exceptions/warnings that we absolutely need then we can create our own alchemlyb.exceptions module. However, I’d like to resist the temptation to invent new exceptions/warnings as much as possible. Less is more, and for users it’s generally better if they encounter exceptions that they are familiar with. You’d have to make a very good case that the existing standard exceptions are not sufficient.

[DrDomenicoMarson]

Yeah, sure, I was thinking about situations where no "standard" exception would be "right", indeed. I'll try to stay away from creating custom exceptions as much as possible, I think they should be easily avoidable!