Do you propose to ban ’throw new RuntimeException(e)’ in all code or just in
tests? The alternative, ’throw rethrow(e)' is only available in test code
(because rethrow lives in TestUtil).
What should people do in non-test code? Continue to ’throw new
RuntimeException’? Use guava’s ’throw Throwables.propagate’? We need clarity.
I don’t see a huge problem here — test error stacks that are a bit hard to read
— so I am not inclined to adopt solutions that would make our overall code
quality worse.
Julian
> On Feb 14, 2019, at 10:26 AM, Stamatis Zampetakis <zabe...@gmail.com> wrote:
>
> Thanks for starting this discussion Vladimir.
>
> Although sneaky throws pattern is a rather hacky way of propagating a
> checked exception it does improve its readability. I like it. Till now I
> never noticed how verbose was a checked exception wrapped in a runtime
> exception.
>
> I agree that the pattern new RuntimeException(e) is a rather bad idea but
> if we forbid this we may need to forbid also new IllegalStateException(e),
> new IllegalArgumentException(e) etc., otherwise developers may turned into
> these as an alternative. I don't know how far we want push this given that
> the new RuntimeException(e) is the number one tool used by developers for
> propagating checked exceptions (even IDEs autocomplete catch blocks adding
> such code).
>
> Overall, I am +1 on this.
>
> Best,
> Stamatis
>
> On Thu, Feb 14, 2019, 8:44 AM Vladimir Sitnikov <sitnikov.vladi...@gmail.com
> wrote:
>
>> 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
>>
