lambdaisland / glogi

A ClojureScript logging library based on goog.log
Mozilla Public License 2.0
119 stars 13 forks source link

Improve ergonomics when no key-value is provided #29

Closed martinklepsch closed 11 months ago

martinklepsch commented 11 months ago
(log/info :test)

Will fail since glogi expects keys and appropriate values. Which makes sense but also isn't a very friendly default maybe?

Would it be a bad idea to pad args to log statements with nil? Or maybe add a clj-kondo linter that will warn people about this within their editor?

plexus commented 11 months ago

I think this breaks down into two separate questions. Do we want to change the signature of the log macros, and are people getting adequate feedback.

Currently the log macros take pairs, and so providing an odd number is an error. I don't think I want to allow arbitrary odd numbers, if you're providing 3 or 5 arguments that's probably a mistake. Providing a single argument... I've had cases where that would be useful. When all I have to log is a keyword "this thing is happening", but no additional data. I tend to pass an empty map in that case for symmetry with other calls, and because sooner or later there might be more data I want to log there.

(log/info :foo/starting {})

But maybe leaving that off should be ok... Some reasons why I don't like the idea too much, it breaks symmetry with pedestal.log (and so we'd need to do additional checking in glogc to make sure you don't do it there), and it opens the door to people doing

(log/info "This is my log message")

i.e. people coming from string-based loggers not realizing that this is somewhat different. And in that case I think it's helpful to fail to make people realize that that's not how you're supposed to use it. So I think I'm going with "no for now".

In terms of feedback, there's always room for improvement. I don't know what error message you currently get exactly, maybe that can be made more clear. Adding a linter seems reasonable, but I don't use linters so I don't have particularly strong opinions about that. If there's something we can include to teach clj-kondo about these macros then I'm fine with doing that.

martinklepsch commented 11 months ago

Hey Arne 🤗

Thanks for the response!

The error I'm currently getting is the one below.

  81 |                         (p/then #(log/info :downloaded-availability-data))
----------------------------------------^---------------------------------------
Encountered error when macroexpanding lambdaisland.glogi/info.
IllegalArgumentException: No value supplied for key: :downloaded-availability-data
    clojure.core/array-map (core.clj:4388)
    clojure.core/array-map (core.clj:4380)
    clojure.core/apply (core.clj:667)
    clojure.core/apply (core.clj:662)
    lambdaisland.glogi/log-expr (glogi.clj:4)
    lambdaisland.glogi/log-expr (glogi.clj:3)
    lambdaisland.glogi/info (glogi.clj:27)
    lambdaisland.glogi/info (glogi.clj:26)
    clojure.core/apply (core.clj:671)
    clojure.core/apply (core.clj:662)
    cljs.analyzer/macroexpand-1*/fn--3704 (analyzer.cljc:4025)
    cljs.analyzer/macroexpand-1* (analyzer.cljc:4023)
    cljs.analyzer/macroexpand-1* (analyzer.cljc:4010)
    cljs.analyzer/macroexpand-1 (analyzer.cljc:4074)
    cljs.analyzer/macroexpand-1 (analyzer.cljc:4070)
    cljs.analyzer/analyze-seq (analyzer.cljc:4107)
    cljs.analyzer/analyze-seq (analyzer.cljc:4087)
    cljs.analyzer/analyze-form (analyzer.cljc:4296)
    cljs.analyzer/analyze-form (analyzer.cljc:4293)
    cljs.analyzer/analyze* (analyzer.cljc:4349)
    cljs.analyzer/analyze* (analyzer.cljc:4341)
    cljs.analyzer/analyze (analyzer.cljc:4369)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze (analyzer.cljc:4362)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze (analyzer.cljc:4360)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/fn--2979 (analyzer.cljc:2358)
    cljs.analyzer/fn--2979 (analyzer.cljc:2354)
    clojure.lang.MultiFn.invoke (MultiFn.java:252)
    cljs.analyzer/analyze-seq* (analyzer.cljc:4080)
    cljs.analyzer/analyze-seq* (analyzer.cljc:4078)
    cljs.analyzer/analyze-seq*-wrap (analyzer.cljc:4085)
    cljs.analyzer/analyze-seq*-wrap (analyzer.cljc:4083)
    cljs.analyzer/analyze-seq (analyzer.cljc:4109)
    cljs.analyzer/analyze-seq (analyzer.cljc:4087)
    cljs.analyzer/analyze-form (analyzer.cljc:4296)
    cljs.analyzer/analyze-form (analyzer.cljc:4293)
    cljs.analyzer/analyze* (analyzer.cljc:4349)
    cljs.analyzer/analyze* (analyzer.cljc:4341)
    cljs.analyzer/analyze (analyzer.cljc:4369)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze (analyzer.cljc:4362)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze (analyzer.cljc:4360)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze-fn-method-body (analyzer.cljc:2147)
    cljs.analyzer/analyze-fn-method-body (analyzer.cljc:2145)
    cljs.analyzer/analyze-fn-method (analyzer.cljc:2169)
    cljs.analyzer/analyze-fn-method (analyzer.cljc:2149)
    cljs.analyzer/fn--2927/fn--2935 (analyzer.cljc:2243)
    clojure.core/map/fn--5935 (core.clj:2772)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:51)
    clojure.lang.RT.seq (RT.java:535)
    clojure.core/seq--5467 (core.clj:139)
    clojure.core.protocols/seq-reduce (protocols.clj:24)
    clojure.core.protocols/fn--8236 (protocols.clj:75)
    clojure.core.protocols/fn--8236 (protocols.clj:75)
    clojure.core.protocols/fn--8178/G--8173--8191 (protocols.clj:13)
    clojure.core/transduce (core.clj:6947)
    clojure.core/transduce (core.clj:6933)
    cljs.analyzer/fn--2927 (analyzer.cljc:2244)
    cljs.analyzer/fn--2927 (analyzer.cljc:2211)
    clojure.lang.MultiFn.invoke (MultiFn.java:252)
    cljs.analyzer/analyze-seq* (analyzer.cljc:4080)
    cljs.analyzer/analyze-seq* (analyzer.cljc:4078)
    cljs.analyzer/analyze-seq*-wrap (analyzer.cljc:4085)
    cljs.analyzer/analyze-seq*-wrap (analyzer.cljc:4083)
    cljs.analyzer/analyze-seq (analyzer.cljc:4109)
    cljs.analyzer/analyze-seq (analyzer.cljc:4087)
    cljs.analyzer/analyze-form (analyzer.cljc:4296)
    cljs.analyzer/analyze-form (analyzer.cljc:4293)
    cljs.analyzer/analyze* (analyzer.cljc:4349)
    cljs.analyzer/analyze* (analyzer.cljc:4341)
    cljs.analyzer/analyze (analyzer.cljc:4369)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze (analyzer.cljc:4362)
    cljs.analyzer/analyze (analyzer.cljc:4352)
    cljs.analyzer/analyze (analyzer.cljc:4360)

Agree that changing the API for this is probably not worth it but I think a better error message (and maybe shortened stack trace) could go a long way in helping users understand what they need to fix.

I think I'll probably find a way to get this flagged by clj-kondo which often gives faster feedback than the compiler.

Cheers :)

martinklepsch commented 11 months ago
;; .clj-kondo/config.edn
{:type-mismatch {:namespaces {lambdaisland.glogi {info {:arities {:varargs {:args [{:op :rest :spec [:any :any]}]}}}
                                                  error {:arities {:varargs {:args [{:op :rest :spec [:any :any]}]}}}
                                                  warn {:arities {:varargs {:args [{:op :rest :spec [:any :any]}]}}}
                                                  debug {:arities {:varargs {:args [{:op :rest :spec [:any :any]}]}}}}}}}

With some help by @borkdude I was able to get this linted as an error 🙂

Screenshot 2023-09-26 at 19 39 11

If you're open to it I could create a PR that packages those linting rules with glogi as per clj-kondo's instructions.

plexus commented 11 months ago

Yeah, that would be cool!

borkdude commented 11 months ago

If the keys are expected to be keywords you could even write [:keyword :any] or [:keyword :string]

plexus commented 11 months ago

They typically are, but I don't think we (or pedestal.log) enforce that.

borkdude commented 11 months ago

alright, then leave it at any