fix NPE in make-log-fn on arity [msg o] when o is nil

[stanislas]

Hi,

I had an NPE as the title of the PR explains. For the arity [msg o], I also remove the string check and put the formatting in an :else (as it is done on arity [msg o1 o2])

what do you think? cheers. stan.

[fzakaria]

Its not evident / clear from the change what was changed. It mostly looks like the conditional was simply re-ordered.

Can you keep the ordering the same to keep the changeset smaller ? Also you'll have to include a unit test.

[rufoa]

I think for consistency with slf4j the default branch when we get o=nil should actually be the Throwable one (fwiw)

There are two overloads defined in the interface for the 2-ary methods:

void info(String format, Object arg)
void info(String msg, Throwable t)

(and we also have to handle the varargs case)

Throwable extends Object so when you pass null as the 2nd arg it wins

[rufoa]

My proposed fix: https://github.com/fzakaria/slf4j-timbre/compare/npe-fix

This fixes the NPE we get when o is nil at runtime.

It does not address the case of o being a null literal at compile time. This is why my test case uses (identity nil) rather than nil - unless we deliberately trick the compiler into letting us resolve the method at runtime by reflection, it fails to compile because the method invocation is ambiguous.

This seems to be an unavoidable edge case with the way java interop works, but I don't see it as a problem because nobody should be passing a null literal to error() etc anyway.

[stanislas]

thanks for the quick answer!

your proposal is much better than mine and solves my problem.

I reordered the clause to actually match void info(String format, Object arg) because I thought that Java would choose it in case of null. Thanks also for fixing my wrong conception of the overloading resolution in Java.

Should I close the PR, now that it is no more necessary?

[fzakaria]

I think so, let @rufoa create a PR from that branch he linked - he's been the main committer on this project for a while.

[rufoa]

yeah I'll deal with it, just need to test something else first

[stanislas]

thanks again guys!