fzakaria / slf4j-timbre

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

fzakaria/slf4j-timbre#54, fzakaria/slf4j-timbre#45: Support SLF4j 2.x, timbre 6.x #61

Closed chrisbetz closed 1 year ago

chrisbetz commented 1 year ago

Hi Farid (@fzakaria),

happy to use slf4j for quite some while now. Now I'm ready to update my projects to SLF4J 2.x and timbre 6.x, so I had to create this PR first:

Replaces the SLF4J 1.7.x Binders with an SLF4J 2.x ServiceProvider. Works in example ("integration-test").

I'd be happy to see this, would simplify my life :) Anyway: Thank you!!

chrisbetz commented 1 year ago

I'm really happy to see that you've taken effort in updating bindings for my favorite logging library! With that said, I do have a few comments.

I removed all the re-formatting, so hopefully, the PR is now easier to check. Sorry for that, it's just my "I don't care for whitespaces" attitude ;) Didn't want to complicate things on your end, though. The actual PR is really small ;) (ah, and don't ask for the magic "2.0.99" number, I copied that from the SLF4J source and it worked.

fzakaria commented 1 year ago

@rufoa has been the maintainer for a very long time. 🙏

rufoa commented 1 year ago

Thank you @chrisbetz for your work on this! And to @PavlosMelissinos @rome-user for your reviews.

It's not clear to me that it will be possible to retain support for slf4j 1.7 emitters, or how much of a problem this will be in practice. I suppose it's more important that we add 2.x support, as your kind PR does, than be paralysed forever by this potential issue.

(And thanks for the timbre 6 support, which I had also intended to add.)

vincentjames501 commented 1 year ago

@rufoa any thoughts on merging this? If you're worried about backwards compatibility maybe this is enough of an upgrade to warrant a 1.x.x release to indicate a potentially breaking change?

devurandom commented 1 year ago

@rufoa, @rome-user: Do you have objections left that prevent merging this? It seems many libraries are updating to SLF4J 2 and to allow slf4j-timbre to be used in that context, it needs to be updated. I supposed slf4j-timbre was stable and mature enough that you can just make a cut, move to version 0.4 and stop supporting SLF4J 1.7? That way, if any bug fix would be necessary for older applications using SLF4J 1.7, and you want to put in the effort to support those, you could release another 0.3 version.

Merging this would close most issues / PRs in this repository:

Closes: https://github.com/fzakaria/slf4j-timbre/issues/42 Closes: https://github.com/fzakaria/slf4j-timbre/issues/45 Closes: https://github.com/fzakaria/slf4j-timbre/issues/54 Closes: https://github.com/fzakaria/slf4j-timbre/pull/59 Closes: https://github.com/fzakaria/slf4j-timbre/pull/60 Closes: https://github.com/fzakaria/slf4j-timbre/pull/62 Closes: https://github.com/fzakaria/slf4j-timbre/pull/63

rome-user commented 1 year ago

No objections. I've approved the changes a while ago, but I am not maintainer of the library.

I have personally moved on to Logback and haven't used Timbre in a while now

rufoa commented 1 year ago

working on this now!

rufoa commented 1 year ago

Please could you all try this out: https://clojars.org/com.fzakaria/slf4j-timbre/versions/0.3.21-10-299a94e-SNAPSHOT

Thank you!

@devurandom @rome-user @vincentjames501 @chrisbetz

dekelpilli commented 1 year ago

@rufoa It seems that there are a few reflection issues here - https://github.com/fzakaria/slf4j-timbre/blob/slf4j2/src/slf4j_timbre/service_provider.clj - all this labels need to be type-hinted (as ^com.github.fzakaria.slf4j.timbre.TimbreServiceProvider)

rufoa commented 1 year ago

Thanks @dekelpilli, fixed in https://clojars.org/com.fzakaria/slf4j-timbre/versions/0.3.21-11-cb46216-SNAPSHOT

vincentjames501 commented 1 year ago

@rufoa , any plans to deploy this?

rufoa commented 1 year ago

yes! will get on it

rufoa commented 1 year ago

Released 0.4.0

https://clojars.org/com.fzakaria/slf4j-timbre/versions/0.4.0

Thanks everyone!