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

chrisbetz · 2023-01-09T15:19:12Z

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!!

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

rome-user

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.

rome-user · 2023-01-12T05:52:31Z

integration_tests/timbre5/project.clj

-  :main example.core
-  :aot [example.core])
+            :main example.core
+            :aot [example.core])


Any particular reason for changing the indentation?

No, auto-format in Cursive and "don't care for whitespace"-setting in git client. No particular reason besides... it's just whitespace. Sorry.

rome-user · 2023-01-12T05:52:40Z

integration_tests/timbre5/src/example/core.clj

-    (.trace logger "Hello from SLF4J")))
+    (.trace logger "Hello from SLF4J")))
+
+
+#_;
+        (-main)


Is the call to -main necessary for the test?

No, it just simplifies testing from a REPL. That's why I made the reader skip (-main) using the "#_" reader macro. The comment is just to ease readability in the editor.

Nothing essential, take it, leave it, it does not matter for functionality.

If you'd rather keep it, you could put it in a rich comment. Makes the intent clearer, compared to that reader macro.

rome-user · 2023-01-12T05:53:14Z

project.clj

-              :plugins [[lein-midje "3.2.2"]
-                        [lein-sub "0.3.0"]
-                        [day8/lein-git-inject "0.0.15"]]
-              :sub ["integration_tests/timbre5"]
-              :aliases {"run-integration-tests" ["sub" "do" "clean," "deps,"
-                                                 "update-in" ":dependencies" "conj" "[com.fzakaria/slf4j-timbre \"lein-git-inject/version\"]" "--" "run"]}}}
+              :plugins      [[lein-midje "3.2.2"]
+                             [lein-sub "0.3.0"]
+                             [day8/lein-git-inject "0.0.15"]]
+              :sub          ["integration_tests/timbre5"]
+              :aliases      {"run-integration-tests" ["sub" "do" "clean," "deps,"
+                                                      "update-in" ":dependencies" "conj" "[com.fzakaria/slf4j-timbre \"lein-git-inject/version\"]" "--" "run"]}}}


I'm assuming nothing is being changed here, but it's hard to tell.

No, just whitespaces. Sorry, again: It's my "don't care" setting.

rome-user · 2023-01-12T05:53:39Z

src/slf4j_timbre/timbre_service_provider.clj

+              #_#_:methods [[getLoggerFactory [] org.slf4j.ILoggerFactory]
+                        [getMarkerFactory [] org.slf4j.IMarkerFactory]
+                        [getMDCAdapter [] org.slf4j.spi.MDCAdapter]
+                        [getRequestedApiVersion [] String]
+                        [initialize [] void]]
+              )
+  )


This should probably be removed since it's all commented out.

Yeah, right.

rome-user · 2023-01-12T05:54:05Z

test/slf4j_timbre/t_adapter.clj

+                        (facts
+                          (timbre/with-context {:foo "preserved"}
+                                               (invoke-each logger ?args)
+                                               (invoke-each logger marker ?args)) => anything ; for side effects only


It's hard to tell if you are just re-formatting code or making changes here.

My last commit introduced several whitespace changes, making it harder to spot the real differences. I cleaned these to simplify comparison. Also removed commented gen-class and obsolete requires in `slf4j-timbre.timbre-service-provider` namespace.

My last commit introduced several whitespace changes, making it harder to spot the real differences. I cleaned these to simplify comparison. Also removed commented gen-class and obsolete requires in `slf4j-timbre.timbre-service-provider` namespace. (second try)

My last commit introduced several whitespace changes, making it harder to spot the real differences. I cleaned these to simplify comparison. Also removed commented gen-class and obsolete requires in `slf4j-timbre.timbre-service-provider` namespace. (third try)

chrisbetz · 2023-01-12T08:57:10Z

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.

rome-user

Your PR looks very clean now! :D
Let's hope @fzakaria can take a look soon

fzakaria · 2023-01-12T13:54:07Z

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

rufoa · 2023-01-15T02:03:10Z

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 · 2023-03-16T17:34:23Z

@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 · 2023-04-03T06:15:37Z

@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: #42
Closes: #45
Closes: #54
Closes: #59
Closes: #60
Closes: #62
Closes: #63

rome-user · 2023-04-03T07:49:37Z

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 · 2023-04-05T03:21:35Z

working on this now!

rufoa · 2023-04-06T00:08:53Z

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 · 2023-04-06T01:54:32Z

@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 · 2023-04-06T02:23:09Z

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

vincentjames501 · 2023-06-05T17:55:04Z

@rufoa , any plans to deploy this?

rufoa · 2023-06-05T17:55:50Z

yes! will get on it

rufoa · 2023-06-13T03:28:19Z

Released 0.4.0

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

Thanks everyone!

fzakaria#54, fzakaria#45: Update to work with SLF4j 2.x, timbre 6.x

66689b4

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

rome-user reviewed Jan 12, 2023

View reviewed changes

chrisbetz added 3 commits January 12, 2023 09:45

Cleanup, see fzakaria#61

d6f9506

My last commit introduced several whitespace changes, making it harder to spot the real differences. I cleaned these to simplify comparison. Also removed commented gen-class and obsolete requires in `slf4j-timbre.timbre-service-provider` namespace.

Cleanup, see fzakaria#61

80b2542

My last commit introduced several whitespace changes, making it harder to spot the real differences. I cleaned these to simplify comparison. Also removed commented gen-class and obsolete requires in `slf4j-timbre.timbre-service-provider` namespace. (second try)

Cleanup, see fzakaria#61

0cb755e

My last commit introduced several whitespace changes, making it harder to spot the real differences. I cleaned these to simplify comparison. Also removed commented gen-class and obsolete requires in `slf4j-timbre.timbre-service-provider` namespace. (third try)

rome-user approved these changes Jan 12, 2023

View reviewed changes

rufoa closed this Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chrisbetz commented Jan 9, 2023

rome-user left a comment •

edited

Loading

rome-user Jan 12, 2023

chrisbetz Jan 12, 2023

rome-user Jan 12, 2023

chrisbetz Jan 12, 2023

PavlosMelissinos Jan 12, 2023 •

edited

Loading

rome-user Jan 12, 2023

chrisbetz Jan 12, 2023

rome-user Jan 12, 2023

chrisbetz Jan 12, 2023

rome-user Jan 12, 2023

chrisbetz commented Jan 12, 2023

rome-user left a comment

fzakaria commented Jan 12, 2023

rufoa commented Jan 15, 2023

vincentjames501 commented Mar 16, 2023

devurandom commented Apr 3, 2023

rome-user commented Apr 3, 2023

rufoa commented Apr 5, 2023

rufoa commented Apr 6, 2023

dekelpilli commented Apr 6, 2023

rufoa commented Apr 6, 2023

vincentjames501 commented Jun 5, 2023

rufoa commented Jun 5, 2023

rufoa commented Jun 13, 2023

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

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

Conversation

chrisbetz commented Jan 9, 2023

rome-user left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PavlosMelissinos Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbetz commented Jan 12, 2023

rome-user left a comment

Choose a reason for hiding this comment

fzakaria commented Jan 12, 2023

rufoa commented Jan 15, 2023

vincentjames501 commented Mar 16, 2023

devurandom commented Apr 3, 2023

rome-user commented Apr 3, 2023

rufoa commented Apr 5, 2023

rufoa commented Apr 6, 2023

dekelpilli commented Apr 6, 2023

rufoa commented Apr 6, 2023

vincentjames501 commented Jun 5, 2023

rufoa commented Jun 5, 2023

rufoa commented Jun 13, 2023

rome-user left a comment •

edited

Loading

PavlosMelissinos Jan 12, 2023 •

edited

Loading