Julian> then there is a real chance that people will instead write
Julian> System.out.println(e);
Isn't System.out banned already?
Of course we would just ban it and that's it.
Julian>new RuntimeException(e), which I don’t think is that bad
It produces garbage in the exception stacktraces (extra meaningless frames,
and a duplicate message which is confusing).
Human-readable exception messages are more important than human-readable
commit messages IMO.
Well, in fact, there are multiple solutions.
Sneaky-throws is a no-brainer that produces even better results than new
RuntimeException(e).
So I think behind the lines of
@defaultMessage RuntimeException(java.lang.Throwable) duplicates
message, so consider use of CalciteResource#rethrow or use an explicit
message
java.lang.RuntimeException#<init>(java.lang.Throwable)
1) Sneaky-throws. It is as simple as catch(Throwable t) { throw rethrow(t);
}
A single catch fits all, and it can even automatically unwrap
InvocationTargetException.
The downsides might include:
1.a) "unexpected" SQLException as a result of a "nothrows" method. Even
though it might be confusing, I think developer more or less expects
SQLException (or alike) when dealing with SQL.
1.b) "missing vital debugging information". For instance, Avatica has
"execute prepared statement" block (see
https://github.com/apache/calcite-avatica/blob/80ccd2007c5d34748852b019f1d1d10ebc94392a/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java#L555-L560
), and it would be great if it printed bind parameters in case of a
failure. Current code just adds "Error while executing a prepared
statement" message that does not really help.
2) Wrap with RuntimeException("", e)
The only "good" thing is the approach is compatible with Java 1.4 way of
throwing exceptions.
The downsides are:
2.a) It is quite verbose as it requires multiple catch blocks that often
repeat each other.
2.b) "missing vital debugging information"==1.b
2.c) Stacktrace is expanded (it includes RuntimeException+frames), and it
adds absolutely nothing meaningful
3) Wrap with RuntimeException(meaninfulMessageThatAddsInfo, e)
This is more or less OK provided the message does not duplicate
e.getMessage()
At the end of the day, e.getMessage() would be printed anyway, so why
duplicate it?
4) Use checked exceptions or
even org.apache.calcite.runtime.CalciteResource. That could even support
localization, however it might require non-trivial efforts to use properly.
For instance use CalciteResources in test code does not pay off. On top of
that, checked exceptions are not compatible with lambdas.
Vladimir
