-
Notifications
You must be signed in to change notification settings - Fork 24
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
fzakaria/slf4j-timbre#54, fzakaria/slf4j-timbre#45: Support SLF4j 2.x, timbre 6.x #61
Conversation
Replaces the SLF4J 1.7.x Binders with an SLF4J 2.x ServiceProvider. Works in example ("integration-test").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
:main example.core | ||
:aot [example.core]) No newline at end of file | ||
:main example.core | ||
:aot [example.core]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for changing the indentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, auto-format in Cursive and "don't care for whitespace"-setting in git client. No particular reason besides... it's just whitespace. Sorry.
(.trace logger "Hello from SLF4J"))) No newline at end of file | ||
(.trace logger "Hello from SLF4J"))) | ||
|
||
|
||
#_; | ||
(-main) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the call to -main
necessary for the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd rather keep it, you could put it in a rich comment. Makes the intent clearer, compared to that reader macro.
project.clj
Outdated
: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"]}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming nothing is being changed here, but it's hard to tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just whitespaces. Sorry, again: It's my "don't care" setting.
#_#_:methods [[getLoggerFactory [] org.slf4j.ILoggerFactory] | ||
[getMarkerFactory [] org.slf4j.IMarkerFactory] | ||
[getMDCAdapter [] org.slf4j.spi.MDCAdapter] | ||
[getRequestedApiVersion [] String] | ||
[initialize [] void]] | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be removed since it's all commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, right.
test/slf4j_timbre/t_adapter.clj
Outdated
(facts | ||
(timbre/with-context {:foo "preserved"} | ||
(invoke-each logger ?args) | ||
(invoke-each logger marker ?args)) => anything ; for side effects only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR looks very clean now! :D
Let's hope @fzakaria can take a look soon
@rufoa has been the maintainer for a very long time. 🙏 |
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.) |
@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? |
@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 |
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 |
working on this now! |
Please could you all try this out: Thank you! |
@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 |
@rufoa , any plans to deploy this? |
yes! will get on it |
Released 0.4.0 https://clojars.org/com.fzakaria/slf4j-timbre/versions/0.4.0 Thanks everyone! |
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!!