search

fzakaria / slf4j-timbre

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

Adapt init to work with v6 of timbre for #67 #68

Open vincentjames501 opened 11 months ago

vincentjames501 commented 11 months ago

I believe this fixes #67 but I admit I'm not positive on the original intent here using the elision env var TIMBRE_LEVEL? It looks like timbre/compile-time-min-level will resolve each of the vars though: 

level
           (or
             (enc/read-sys-val "taoensso.timbre.min-level.edn" "TAOENSSO_TIMBRE_MIN_LEVEL_EDN")
             (enc/read-sys-val "TIMBRE_LEVEL")     ; Legacy
             (enc/read-sys-val "TIMBRE_LOG_LEVEL") ; Legacy
             )
rufoa commented 10 months ago

Thanks for your work on this @vincentjames501!

dissocing :_init-config seems like a sensible fix for that part of the problem.

It was never a great solution to compare *config* and timbre/example-config to identify when timbre has been configured, and this brittleness has now caught us out. I wonder if there is a better way to fix this in future.

IIRC, the reason I used (var-get (resolve 'taoensso.timbre/example-config)) rather than simply taoensso.timbre/example-config is because I needed it to resolve at runtime to the example-config in the version of timbre your project is using. This might not be the same as the version of timbre that slf4j-timbre is compiled against. So I think we probably need to leave that part in?

Re. @#'timbre/compile-time-min-level: this would definitely be neater (thanks for pointing it out) but I'll have to think about whether this gives us the behaviour we want. A problem we had previously is that, similar to the example-config thing above, compile-time elision seemed to be impossible with slf4j-timbre because it always happened at compile-time of slf4j-timbre (i.e. when I ship the library) rather than at compile-time of your project (which is what you'd want). I'm afraid I don't recall whether I ever got this part to work correctly.

vincentjames501 commented 10 months ago

@rufoa , feel free to modify this MR as you see fit. This does fix it for us locally while still allowing changes at runtime. It may make sense to just bump the major version of this package and just say this is only usable for v6 of timbre?

vincentjames501 commented 8 months ago

@rufoa , can we collab a bit more on getting this one in? Would bumping the major version of this project help here while saying it's only compatible with v6 of timbre?

rufoa commented 8 months ago

Sorry, things have been very hectic lately. I'll sort it out this weekend. I agree that we can ditch support for timbre 5 and bump our version correspondingly

vincentjames501 commented 8 months ago

I COMPLETELY understand and thank you for your work on this!

rufoa commented 7 months ago

Hi @vincentjames501,

I've just pushed version 0.4.1 to clojars, containing just the dissoc fix. I think this should be enough to fix your issue (but please let me know if not). It's actually compatible with timbre 5, but I agree we shouldn't be concerned about dropping that.

I'll look into the LOG_LEVEL and compile-time elision stuff separately, because I'm not convinced that's ever actually worked properly, and will have to familiarise myself with the code again.

Thanks again for your patience and understanding while I got round to working on this!

vincentjames501 commented 7 months ago

@rufoa , thanks! So far it's looking good.

ivarref commented 5 months ago

Thanks for the good work @vincentjames501 and @rufoa

Using 0.4.1 solves this problem here as well. I can also change the levels at runtime/REPL, no problem.

ivarref commented 5 months ago

And cc @vemv who opened the original issue back in 2018. (We just updated timbre and got this bug.)