Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vincentjames501
Copy link

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
Copy link
Collaborator

rufoa commented Aug 19, 2023

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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Collaborator

rufoa commented Oct 19, 2023

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
Copy link
Author

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

@rufoa
Copy link
Collaborator

rufoa commented Nov 20, 2023

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
Copy link
Author

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

@ivarref
Copy link

ivarref commented Jan 25, 2024

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
Copy link

ivarref commented Jan 25, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression for timbre v6
3 participants