fzakaria / slf4j-timbre

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

Less java #7

Closed rufoa closed 8 years ago

rufoa commented 8 years ago

The adaptor/factory code is much neater if written in pure clojure rather than split in half with java interop.

I've also added a few tests for the factory class

fzakaria commented 8 years ago

I've added you as a collaborator @rufoa since you've added now a good amount of the code.

I had some questions about the code but mostly for learning purposes.

fzakaria commented 8 years ago

Why doesn't something like this work

(defn slf4j-bridge
[log-fn]
    (fn
        ([msg]
            (log-fn msg))
        ([msg o1 o2]
            (let [ft (MessageFormatter/format msg o1 o2)]
                (log-fn (.getThrowable ft) (.getMessage ft))))
        ([msg o]
            (cond
                (string? o)
                (let [ft (MessageFormatter/format msg o)]
                    (log-fn (.getThrowable ft) (.getMessage ft)))
                (.isArray (class o))
                (let [ft (MessageFormatter/arrayFormat msg o)]
                    (log-fn (.getThrowable ft) (.getMessage ft)))
                (isa? (class o) Throwable)
                (log-fn o msg))))
)

(def -error (slf4j-bridge #'timbre/error))
(def -warn  (slf4j-bridge #'timbre/warn))
(def -info  (slf4j-bridge #'timbre/info))
(def -debug (slf4j-bridge #'timbre/debug))
(def -trace (slf4j-bridge #'timbre/trace))
rufoa commented 8 years ago

nitpick: Does dosync use locks under the hood ? It might be not equal to a non-locking concurrent hashmap.

As I understand it, ConcurrentHashMap does use (per-bucket) locks for writing. dosync/refs use the STM and the STM also uses locks under the hood. They are the same in that they both allow thread-safe access to the map.

I've changed it to use an atom rather than a ref because we don't need coordinated access, meaning we can get away with using spinloops rather than the STM. This further reduces the reliance on locks.

Realistically though, we're not going to get high lock contention in this bit of code anyway, so I wouldn't worry too much about it.

rufoa commented 8 years ago

Why doesn't something like this work

timbre/error etc are macros and you can't take the value of a macro and pass it to a function like you could if they were functions.

If timbre/error took one argument we could get around this problem by passing #(timbre/error %), or if it took exactly two arguments we could pass #(timbre/error %1 %2) etc, but it's actually variadic so this trick doesn't work.

We'd instead have to use eval which is pretty horrible.

The easier solution is to make the bridge itself a macro.

rufoa commented 8 years ago

I've added you as a collaborator @rufoa since you've added now a good amount of the code.

Thanks, I appreciate it!