Parameterize the println that informs of the fallback cases

[vemv]

Hi there!

I was finding the (println "[PINPOINTER] Failed to analyze the spec errors, and will fall back to s/explain-printer\n") somewhat noisy and with limited usefulness.

This PR leaves up to the user whether to run it (by passing an empty fn), or to actually log the incidence in detail: it can be very handy to access the underlying exception, that way one can try to fix it.

You can try out these changes by raising an error (e.g. https://github.com/athos/Pinpointer/issues/2) with the option being passed.

Cheers - Victor

[athos]

Hi, @vemv! Thank you for the feedback 😄 And I’m sorry for my slow response.

I agree that that warning message can be somewhat noisy, so improvements around it are very welcomed.
OTOH I’m also considering some other possible options than this PR proposes since I’m not sure if it is that likely that users want to customize “how the warning message is printed” or “what is printed as the warning message”.

I feel more like users would just want to disable showing that message, eg.:

(p/pinpoint spec input {:fallback-on-error :disable-warning-message})

Or, to give users more liberty of controlling the behaviors on error, the following might seem more intuitive:

(p/pinpoint spec input {:on-error (fn [ed err] (println “Something bad happened!”) (s/explain-printer ed))})

What do you think of these? Do you have any concrete use case of this change in mind?

[vemv]

Hey there @athos ! Thanks for the reply.

(p/pinpoint spec input {:fallback-on-error :disable-warning-message})

An option which can have nil, true, false or :disable-warning-message values seems overloaded to me. Even more if we introduced e.g. :print-exceptions in a future (for making the default logger print the exceptions).

(p/pinpoint spec input {:on-error (fn [ed err] (println “Something bad happened!”) (s/explain-printer ed))})

I like this option better. But, one has to remember to call s/explain-printer, which is an implementation detail of both clojure.spec and pinpointer.

It's a cost I personally could assume though, no biggie.

Do you have any concrete use case of this change in mind?

Primarily, being able to debug things (since no exceptions are logged). Also, the warning message is not tremendously useful to me, as I can guess when is Pinpointer being used and when not.

Also, the message happens to break some unit tests we have.

Cheers - Victor

[vemv]

Hey there,

kindly let me now what you think when you have a chance.

Thanks!