Clojurians Log v2

@bbrinck Thanks for looking into this for me. What is it that is currently defining that fdef?

I can’t seem to find it in expound; or kaocha

ahh I see it now https://github.com/bhb/expound/blob/e90a16162e97d16904d2353f46084e139b0e8f37/src/expound/alpha.cljc#L1030

So I guess the reason this happens and hasn’t been discovered before is because expounds printer assumes it never needs to print success messages? i.e. the binding approach doesn’t currently assume that users are going to call s/explain-str themselves within that binding form?

@rickmoynihan I suspect the reason this hasn’t been reported earlier is that you need 3 things to be true. 1. You must use explain-str (not expound-str) 2. You must have a case where you are calling explain-str for data that matches the spec (a common use case is to only call this if s/valid? returns false) 3. You must have instrumented expound/printer I suspect that’s quite rare

;; Using expound directly works
  (expound/expound-str :foobar/name "")
  ;; => "Success!\n"  

  ;; A common pattern of using valid? + explain-str works
  (binding [s/*explain-out* expound/printer]
    (if-not (s/valid? :foobar/name "")
      (s/explain-str :foobar/name ""))
    )
  ;; => nil

  ;; If instrumentation is off, it works
  (binding [s/*explain-out* expound/printer]
    (s/explain-str :foobar/name "")
    )
  ;; => "Success!\n"  )

  ;; But if instrumentation is turned on, it fails!
  (st/instrument)
  (binding [s/*explain-out* expound/printer]
    (s/explain-str :foobar/name "")
    )
  ;; error

  ;; Even w/ instrumentation, expound-str works
  (expound/expound-str :foobar/name "")
  ;; => "Success!\n"

  ;; ... as does pattern of using valid? + explain-str

  (binding [s/*explain-out* expound/printer]
    (if-not (s/valid? :foobar/name "")
      (s/explain-str :foobar/name ""))
    )
  ;; => nil

Yeah I suspect it is quite rare… The code in question here was actually a clojure.test assertion that some test data conformed to the clojure spec…

(= "Success!\n" (s/explain-str :mut.download/job-instance-schema test-job))

an s/valid? call just gives a true/false error etc.

Instrumenting is a better approach generally to this kind of thing too… but there are some problems with instrumentation in this code base. Mainly that we use integrant and load-namespaces quite extensively; so not all specs in use are loaded upfront; because the code loading is dynamic.

I think the following would work

(is (s/valid? :foobar/name 1)
        (s/explain-str :foobar/name 1)
        ))

(basically test for valid but then use explain-str for the is failure message)

hmm nice idea

I guess that’ll still cause the expound problem though 🙂

It doesn’t seem to

I haven’t dug into the clojure.test code, but my suspicion is that the message is not evaluated unless is check fails

yeah I guess the messages are evaluated later outside the binding

Oh, nevermind. I still had the “workaround” spec declared. The above won’t work 😞

I suppose you could use a macro to delay eval of the message but at that point it’s getting pretty complicated

can we not just correct the printer spec in expound; to allow nil? Or does that imply having the printer print success messages?

i.e. your workaround; I’m curious why that’s not a fix.

It is the fix

I just haven’t had time to apply it and cut a release 🙂

ahh cool

ok, well I can wait. It’s not a big deal.

I’d already commented out the assertion for now anyway.

FWIW, I haven’t used it, but there are also fancier tools to assert that some data matches a spec. YMMV https://github.com/nedap/utils.spec/blob/3c9c0f32d3bb436aa8cc6b975b3d843ca95968ab/src/nedap/utils/spec/api.cljc#L9-L15

ahh thanks good to know — this code is pretty old. I had written similar things in the past, but never extracted it into something to reuse as it was always a bit hacky.

and made do with the string check in this instance

Yeah, it makes sense. The string check is a good solution - if there wasn’t this bug 😆 . In any case, thanks for reporting it, I’ll try to get it fixed soon.

Well thanks a million anyway, it was insightful 😁