Timber object should also support string arguments

[bj0]

Timber object should also support string arguments

[bensandee]

I would argue that these are not idiomatic for Kotlin and that they would introduce extra ambiguity when it comes to code completion. Including them would expose methods that are inherently less safe as well, as referenced in the readme section about lint checks.

The only reason I'd use the old syntax would be for compatibility while migrating code but I would prefer to do that one module at a time any ways.

[bj0]

It's hard to argue it's not idomatic for Kotlin when that's exaclty what Anko does: https://github.com/Kotlin/anko/wiki/Anko-Commons-%E2%80%93-Logging

Not sure about safety, I was just thinking more for people who are use to using systems like Anko, it would make it cognitively easier to use. I currently just create my own.

[bensandee]

Personally, I don't use or like Anko. It's popular but that doesn't make it idiomatic Kotlin. But whatever, I don't care either way. Maybe patches welcome, eh?

[bj0]

I also don't use Anko anymore, but I only meant that since there's no specific word on it in https://kotlinlang.org/docs/reference/idioms.html, than official kotlin/jetbrains libraries like Anko are going to be most commonly referenced as what's 'idiomatic'. I can look into creating a patch with my extensions if there's interest beyond just me.

[bensandee]

I think that how Kotlin handles check() and require() are great examples of how logging should be handled idiomatically. I fail to see how parametric substitution methods offer anything of value beyond the inline string substitutions and like I said, it's easier to make mistakes (wrong number of arguments, etc) and catching those requires lint checks, etc.

That said, I feel like the whole point is moot. One fatal flaw for this lib at the moment is that logging messages are fully realized and rendered any time a Tree is plant()ed. In my reading of the code, it does not matter if the log message will actually be logged or not, based on the underlying priorities. This is substantially different behavior from Timber.

(updated w/clarifications)

[bj0]

I didn't have parametric substitution in mind, only basic no-arg strings like d("this method called"). For anything else I would use a lambda.

I didn't notice the log check before, good point. And looking at Timber, since isLoggable is per-tree, you couldn't really support passing in lambdas that deep without rewriting timber in Kotlin.

[bensandee]

Even with that addition it is easy to make a mistake if you're not paying attention. If your log message inadvertently has inline string subs (a situation that should be very common and expected in Kotlin), the method invocation that takes a string will cause the log message to be built regardless. So even if underlying issue discussed earlier is fixed, this is still a potential programming issue -- particularly for people who don't have a firm grasp on lambdas vs strings.

You could add these as a companion functions without changes to this lib:

inline fun TimberKt.d(message: String) = Timber.d(message)

[ajalt]

Thanks for the detailed discussion! I would prefer not to add another overload for all the functions, especially when the new version is error prone.

@tbsandee Version 1.1.0 didn't call the log lambda unless the message was actually logged. However, since Timber doesn't expose isLoggable, the implementation used non-inline lambdas. If you're ok with that and need the precise behavior, that version works fine.