dialog icon indicating copy to clipboard operation
dialog copied to clipboard

[#18] Provide slf4j 2 compatibility - implement service loader

Open ieugen opened this issue 3 years ago • 3 comments

Fixes #18

  • Implemented service loader
  • All tests pass

TODO (need help):

  • Check service loader implementation
  • Restore the older logging bridges - jul and commons logging

ieugen avatar Nov 04 '22 10:11 ieugen

@greglook : I've fixed the check for slf4j version but I ran into another one. I need your help figuring this one out.

Found uncommented reference to 'requiring-resolve' in logger classes:
src/java/dialog/logger/DialogServiceProvider.java:        IFn resolve = RT.var("clojure.core", "requiring-resolve");

So this check is failing because I put the Service Provider implementation in dialog/ dir. I took the code from the previous StaticLoggerBinder implementation which was in a different path. The other path was not searched by test/check-dynamic-resolves

readonly MATCHES=$(grep -RE '^ *IFn.*"requiring-resolve"' src/java/dialog)

Why is the check here anyway? I don't know what solution to adopt in fixing this.

ieugen avatar Nov 15 '22 10:11 ieugen

I chose to move the service provider class to package dialog.impl and update the check to look into dialog/logger

ieugen avatar Nov 15 '22 11:11 ieugen

The purpose of that test is to make sure that Dialog doesn't ship with a significant performance penalty that is caused by a development-time affordance. In the normal case in the current implementation, the StaticLoggerBinder is responsible for loading the dialog namespaces and resolving the various function vars which the Java code needs to hook into Clojure: https://github.com/amperity/dialog/blob/3933260fec9c88068cf091aa47d6fd8999194499/src/java/org/slf4j/impl/StaticLoggerBinder.java#L53-L63 This should only happen once in a real deployment with dialog, because locking the namespace loading monitor and re-resolving the var on every single logging call would be extremely expensive and slow. Those functions are then passed to the DialogFactory and then into each DialogLogger: https://github.com/amperity/dialog/blob/3933260fec9c88068cf091aa47d6fd8999194499/src/java/org/slf4j/impl/StaticLoggerBinder.java#L69-L70 https://github.com/amperity/dialog/blob/3933260fec9c88068cf091aa47d6fd8999194499/src/java/dialog/logger/DialogFactory.java#L57-L58 However, as you can see, loggers are cached, so when you're actually developing Dialog itself, this can be really annoying as it causes you to need to restart the REPL process to pick up changes, even to Clojure code. So there are some commented-out blocks like this, which you can enable to make hot-reloading work again, while you're working on Dialog. https://github.com/amperity/dialog/blob/3933260fec9c88068cf091aa47d6fd8999194499/src/java/dialog/logger/DialogLogger.java#L130-L133 The test script is ensuring that these blocks are never left uncommented on accident.

greglook avatar Nov 15 '22 16:11 greglook

hi @greglook . we are using apache jena in our project and it's bound to slf4j-api 2.0. I'm going to look into using dialog with the API for our project (was on todo for some time).

Any chance we can get this PR merged in dialog? More and more projects will most likely move to slf4j-api 2.0 since there are really no downsides. And the API is backward compatible. Just need to make sure any implementations for < 2.0 are on the classpath.

ieugen avatar Feb 01 '23 14:02 ieugen

Codecov Report

Base: 94.62% // Head: 94.62% // No change to project coverage :thumbsup:

Coverage data is based on head (d656952) compared to base (060a5f9). Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #19   +/-   ##
=======================================
  Coverage   94.62%   94.62%           
=======================================
  Files           8        8           
  Lines         614      614           
  Branches       13       13           
=======================================
  Hits          581      581           
  Misses         20       20           
  Partials       13       13           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Feb 01 '23 15:02 codecov-commenter