search

fzakaria / slf4j-timbre

SLF4J binding for Clojure's Timbre
Eclipse Public License 1.0
94 stars 23 forks source link

Respect TIMBRE_LEVEL system property, default to log level to :info #36

Closed ivarref closed 4 years ago

ivarref commented 4 years ago

Respect TIMBRE_LEVEL system property, default to log level to :info before timbre end user has had the chance to configure log level. Ref issue #32

rufoa commented 4 years ago

Thanks for this! Will look into it asap.

On Tue, 3 Dec 2019, 13:43 Ivar Refsdal, notifications@github.com wrote:

…efore timbre end user has had the chance to configure log level. Ref issue #32 https://github.com/fzakaria/slf4j-timbre/issues/32

You can view, comment on, or merge this pull request online at:

https://github.com/fzakaria/slf4j-timbre/pull/36 Commit Summary

  • Respect TIMBRE_LEVEL system property, default to log level to :info before timbre end user has had the chance to configure log level. Ref issue

    32

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fzakaria/slf4j-timbre/pull/36?email_source=notifications&email_token=AAOHHKBT4KLOBMZSM6OL5WDQWZO6TA5CNFSM4JUYHC52YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H5VSY3A, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOHHKBRWZ52Y7TPZZG4P6DQWZO6TANCNFSM4JUYHC5Q .

ivarref commented 4 years ago

Great, hoping for a positive review (and if so a new release)!

rufoa commented 4 years ago

Changing the default level to :info for slf4j-originated messages is sensible and it would bring us in line with the default logging level of SimpleLogger in slf4j. So a user who adds slf4j-timbre to their project wouldn't suddenly get more logs than they previously did from slf4j by default. :+1:

However, timbre's default log level is :debug. So this change would also inadvertently silence :debug level messages which don't originate from slf4j. I don't think this unintended side effect is ideal, if we can reasonably avoid it. Including slf4j-timbre shouldn't change the global behaviour of timbre in situations that have nothing to do with slf4j.

I wonder if there is a way we could restrict the change of level to only our own log messages somehow. If I can't think of something clever, I'll merge this as it currently stands. Thanks again.

rufoa commented 4 years ago

@ivarref please could you test [com.fzakaria/slf4j-timbre "0.3.15-SNAPSHOT"] for me?

I changed your code a little and ended up with this: https://github.com/fzakaria/slf4j-timbre/commit/10ca6f0f58b85fda674c0f55e4a1b2c1c38a629f

What do you think? It's a little idiosyncratic, but restricts the impact of our changes to only our own timbre calls, and only until a proper timbre config is set by the end user.

rufoa commented 4 years ago

cc @runejuhl: this should be better than what you currently do with :ns-blacklist, if you're able to give it a test? Thanks

"0.3.15-SNAPSHOT"

ivarref commented 4 years ago

Hi @rufoa

I've tested your solution and it works fine!

I think your solution is a very good one, particularly that it does not break existing behaviour in tufte (like mine did). Excellent job!

rufoa commented 4 years ago

Thanks! Pushed to clojars as "0.3.15"