Deal with an unhandled exception in small molecule transition list reader…

[bspratt]

… by making exception handling more consistent throughout

[brendanx67]

This overwrites the stack of the original exception. Not a good idea. These are only 3 types of exceptions. Better to catch them all and be slightly redundant than to do this and blow away the stack of a programming error exception.

[nickshulman]

I am not sure that it makes sense to have "ThrowIfNotParserError".

Specifically, anytime that Skyline knows which line of the transition list the exception happened on, then the error is a parser error, and should be displayed to the user as a message associated with that line of the transition list.

That is, even if the error is something which is probably a bug in our code, such as "NullReferenceException", the user still should be told which line of their transition list caused us to crash. Similarly if it was "out of memory" or "error reading from file".

It's like when we get "Compile error on line XXXX: Internal compiler error". There might not be anything syntactically wrong with that line of code, but it's still super helpful to know which line of our source code caused the compiler to crash.

[brendanx67]

Interesting thought, Nick. That might justify wrapping it in a new exception that contains as much contextual information as possible. It is still a tricky call whether we would want to know about the NRE or hand it to the user to try to find a workaround and maybe never tell us.

[brendanx67]

Nice that you can use a function in that when clause. I was not even aware of this option. I think it beats Nick's use of IsProgrammingDefect() inside the catch clause and then WrapAndThrow() for the exceptions that will not be handled. This simply preserves the unhandled exceptions unchanged. Nice.

[bspratt]

Nice that you can use a function in that when clause. I was not even aware of this option. I think it beats Nick's use of IsProgrammingDefect() inside the catch clause and then WrapAndThrow() for the exceptions that will not be handled. This simply preserves the unhandled exceptions unchanged. Nice.

Yes, I was pleased to discover that - I wasn't looking forward to a bunch of code duplication.

[nickshulman]

Nice that you can use a function in that when clause.

For the record, though, if you had to do this using the older C# syntax, it would have been:

catch (Exception exception) {
  if (IsParserException(exception)) {
    errmsg = ex.Message;
  } else {
    throw;
  }
}

That "throw" there (as opposed to "throw exception") preserves the exception's original stack trace as if it had never been caught.

[bspratt]

Yes, the key thing about preserving the call stack is that the bare throw is in the catch block. But nice to have the newer more compact form available.