proposal : defmsg equivalent for arbitrary predicates

[mpenet]

Hi,

I know that right now we can do this for things that are referencable via a qualified-kw.

Would you be open to add the ability to replace the output from predicates in messages?

The idea would be to add pattern matching on predicates and allow to generate a message from the bound values for a match:

from a (s/coll-of string? :min-count 3)

should satisfy

  coll?

Could become

should be a Collection with at least 3 elements

string? -> should be a string and so on.

It's quite easy for just predicates, it gets a bit more complicated for sexprs.

I have done something like that in an internal lib at work using meander, it matches using definitions look like this:

    ;; int-in
    [(clojure.spec.alpha/int-in-range? ?min ?max %)
     (format "should be an Integer between %s %s"
             ?min ?max)]

Then it's all turned into a big meander m/match, which has pro/cons (it's a macro so it requires some juggling, also it think it won't scale to very large match numbers, we might hit the method size limit on the jvm).

There is also https://github.com/alexanderkiel/phrase/ which allows something similar via other means.

Anyway, food for thought, but I think that could be a nice addition.

[mpenet]

I am toying with these ideas in https://github.com/mpenet/expound/tree/feat/better-messages-errs (didn't write tests yet)

While doing so I already improved defmsg to allow to resolve aliases (as deep as they are).

(s/def ::foo (s/keys :req [:bar]])
(s/def ::bar ::bar2)
(s/def ::bar2 string?)

(defmsg ::bar2 "should be a bar2")

(explain ::foo {:bar 1}) -> "[...] should be a Bar2 [...]"

I also added the ability to create display names for specs (wip), typically it's nicer to say "String" than "string?" to a end-user if it's publicly exposed, or allows to override the display of giant sexprs (since it's resolved up to the s/form in master), with nice wording :

(defspec-name ::bar2 "A Bar2")

(expound ::foo {})

should contain keys: :bar2, :baz

| key   | spec    |
|=======+=========|
| :bar  | A Bar2  |
|-------+---------|

The next step from my initial goal would be to allow to generate custom error messages for spec forms (not just idents):
So when there is no custom name/message for a thing we can at least decipher the meaning of the forms if the pattern is defined somewhere (in the registry), a bit like phrase allows.

In any case, I could split these in 3 different prs if we can agree on having these added in one form or another.

[mpenet]

lastly one easy solution to the predicate translation task I mentioned would be just to allow the use of phrase in expound (it's a tiny lib).

[bhb]

@mpenet Thanks for the idea! I'll think this idea over and let you know.

[mpenet]

great! I was thinking, if I modify slightly the code in my branch for "traversing" defmsg it could handle custom messages for things like string? (any qualified-ident?) too.

But maybe there's a need to step back and see how to unify all this, it feels like there is some overlap between these features.

If I was to go nuclear I would probably design a new version of defmsg (with a new name) that can handle messages for both idents and arbitrary sexpr from problem.pred, something akin to what's in phrase and deprecate defmsg.
That mean it would take a single function as input that would attempt to match the argument it gets and return a new message. It would also have to handle "traversing" aliases.

That way the end-user can choose to user whatever (s/conform, meander, raw clojure), and group or not some patterns/formats.

(set-msg-formater! (fn [pred] (when (= `string? pred) "Should be a string"))
(set-msg-formatter (fn [pred]  (when (= ::foo pred) "should be a foo"))
(set-msg-formatter! (fn [pred-form]
                      (when (= (first pred-form) `int-in-range?)
                        (format "should be an integer between %d and %d"(second form) (nth form 3})))
(set-msg-formatter! (fn [pred]
                      ;; coll-of opts
                      (meander/match pred
                                     (<= ?min-count (count %) Integer/MAX_VALUE)
                                     (format "should contain at least %s elements"
                                             ?min-count)

                                     (<= 0 (count %) ?max-count)
                                     (format "should contain at most %s elements"
                                             ?max-count)

                                     (<= ?min-count (count %) ?max-count)
                                     (format "should contain between %s %s elements"
                                             ?min-count ?max-count)
                                     (<= 0 (count %) ?max-count)
                                     (format "should contain at most %s elements" ?max-count))))

etc...

Then it's a bit naive as it would try in order, alternatives would be to specialize matchers for common cases, spec keys, simple symbols and preds. Or use something like meander/phrase to compile all these to an efficient matcher.

For the spec naming feature that would be something separate.

[bhb]

@mpenet Can you walk me through the problem you're running into?

For instance, in your case, are you in control of writing the specs? Or are you improving the printing for specs you don't control?

I'm specifically wondering if it's possible just provide your specs a name, which would then allow you to provide custom descriptions. As a bonus, giving the specs name might make them more reusable and clarify their meaning.

e.g.

(require '[expound.alpha :as expound])
(expound/expound
 (s/coll-of string? :min-count 3)
 []
 {:print-specs? false})
;; -- Spec failed --------------------

;;   []

;; should satisfy

;;   (<= 3 (count %) Integer/MAX_VALUE)

;; -------------------------
;; Detected 1 error


;; Wrap the spec in a more meaningful name to fix ... 
(s/def ::tags (s/coll-of string? :min-count 3))
(expound/defmsg ::tags "should be a Collection with at least 3 strings")
(expound/expound
 ::tags
 []
 {:print-specs? false})
;; -- Spec failed --------------------

;;   []

;; should be a Collection with at least 3 strings

;; -------------------------
;; Detected 1 error

[mpenet]

Sure.

We would like to avoid exposing clojure sexprs in the error messages and the descriptions of shape of maps for instance (and likely other places). Basically make everything more human readable (without clojure experience)

For instance, right now if you are missing some keys in a map, the ascii table will contain the raw predicate associated to that key and not a human readable message (even if there's a defmsg associated), in this case we're not really interested by an error message per say, but more likely a way to associate a display text to a spec saying what it represents.

Another example is the coll-of with :min-count, it would be nice for the error of "parameterized" specs (all derivatives of every, inst-in, int-in, our own string-of etc) to be generic.

But that's just one of these issues.

If I step back and answer to your questions directly, we have a mix of specs we control and some we dont, we would like to avoid creating intermediary specs to just make error messages better.

I can give you a concrete example, let's say you have a spec for a ::port, and that spec is re-used with other keys in other context, ::cassandra-port ::metrics-port ::http-server-port that are aliases to ::port ((s/def ::metrics-port ::port)), even if you have a defmsg on ::port, the other keys that are just aliases to it will not use that upon errors. There is no support for what I call traversing aliases right now. My experimental branch I mentioned earlier adds this; so if you have an error on ::cassandra-port it will get the defmsg of the closest spec, so in that case ::port.
That alone would be quite a nice addition in my opinion and save us from writing dozens of defmsgs for essentially the same thing.

So there are a few distinct thing we're trying to solve:

• we should be able to describe things, human readable (usable for many contexts, potentially raw errors too, but also in spec display such as the s/keys ascii table, ex replace something like string? by "String")
• we should allow defmsg to resolve aliases values
• we should allow to associate a "parameterizable" message to a spec values and potentially provide good defaults (descriptions and error messages) for the common cases (coll-of/map-of etc), if that part would work the first issue can be implemented from it.

I think there must be basically a clear distinction between the human consumable description of a spec and an error message associated to that spec, even tho the later could be derived from the former (by default). Right now defmsg is only good at doing error messages, you could not reuse its value to improve the s/keys display ascii table in case of a missing key error for instance.

[mpenet]

I created https://github.com/bhb/expound/pull/234/files that demonstrate the modifications of defmsg to make it traverse all specs associated with key until a match is found and also as a result make it able to handle any type of spec form.

[bhb]

Thanks a lot for all the great context and detail! I’ll look all this over later this week.

[bhb]

@mpenet Thanks for the write-up and the code! I appreciate it!

we should allow defmsg to resolve aliases values

I agree. At first glance, your implementation looks good, so I'm inclined to add some version of this feature. Thanks!

we should allow to associate a "parameterizable" message to a spec values

While I can see why this would be convenient, I'm not convinced the complexity of adding this feature would justify it's utility. In practice, do most codebases have many different uses of coll-of, map-of, etc, but also don't have semantic spec names? If we fixed the "resolve aliases" issue, I think users could workaround this by creating their own macro for declaring coll-of specs plus messages.

This is a "no for now" decision, but please feel free to create a separate issue for this where you can lay out the benefits. I'll probably close the issue for now, but then we would have a record of the decision and I'd welcome additional comments from the community if people are feeling this pain 😄

and potentially provide good defaults (descriptions and error messages) for the common cases (coll-of/map-of etc)

If I did this, I'd definitely want to make it optional and not the default. It's not clear to me that providing descriptions ("should be a Collection with at least 3 elements") are preferable to showing the spec form itself (should satisify (s/coll-of string? :min-count 3)).

When I'm considering any change to Expound, my primary criteria is "will this change help developers resolve their Spec error faster or more easily?" Spec is a predicate-based system, and developers who are using spec understand this. It may actually be harder to quickly parse a natural language description than the spec itself.

(I hold this opinion because I believe the users of Expound are developers, not end-users. For end-users, I think systems like Phrase that hide the predicates are really great)

we should be able to describe things, human readable (usable for many contexts, potentially raw errors too, but also in spec display such as the s/keys ascii table, ex replace something like string? by "String")

For the reasons above, it's not clear to me that a human-readable spec description is better than the spec form, at least not enough to justify expanding the Expound API at this time. Again, I'm happy to track this in an issue, so I can document the decision and potentially revisit later.

---

As an aside, I've had a few different requests from users to vary the output of Expound in some way. Like your use cases above, these are all very legitimate but often don't warrant a special extension point or expanding the API.

My gut is that the core problem is that the output of Expound is just a string. For awhile, I've wanted to refactor the code base so that Expound could return a data structure which represents the entire error message. Users could then manipulate this for whatever use case they had and then pass it along to a function that would "render" the final string.

https://github.com/bhb/expound/blob/master/doc/arch/adr-003.md

I took a look at this for a bit but got stuck and haven't come back to it. If it's something that would interest you, I'd certainly be open to ideas or code for this problem.

[mpenet]

I agree. At first glance, your implementation looks good, so I'm inclined to add some version of this feature. Thanks!

That's great news, that's the number one feature we need on our list of priorities.

About extension points I agree completely. Instead of baking our specific use case opening a few extension points could be a good way to let users achieve this.

Then the main question would be at what level would you want to do this. Do you want the users to have full control over what's displayed (essentially writing an explain printer themselves) or just open a few doors into the parts that could be of interest. I think in most cases people are after changing some small/limited part of the whole output (like we are), so I think a few hooks could be good enough.
I could imagine options such as :spec-key-handler :pred-handler :value-handler, and possibly more for a high level api, basically just middlewares into the datastructure -> string transformation for these values in various contexts. This way you can cherry pick what you want to modify without having to do a deep dive into the whole thing.
And maybe something lower level where you expose the raw internal data-structures (basically what you get after problems/annotate) and let the user define his own printer, but that's kind of possible today already I think, it's just not documented.

I've wanted to refactor the code base so that Expound could return a data structure which represents the entire error message

That would be a really good thing to do indeed. Our internal lib that deals with this stuff took this approach, but we just extend the explain-data with extra information. I think it's kind of what you do as well, I wouldn't depart too much from the original explain-data format personally.

(I also think defmsg should have an equivalent that just takes a function for all inputs, set-msg-fn! or something, that take as input the form that caused the failure. From there you can plug anything for rendering (meander, s/conform, etc))

[bhb]

Then the main question would be at what level would you want to do this. Do you want the users to have full control over what's displayed (essentially writing an explain printer themselves) or just open a few doors into the parts that could be of interest.

It's a good question. Ideally, I would identify a few extension points, but in practice, the requests have bee quite varied

• Change the way specs are displayed in tables (this issue)
• More generic control over spec messages e.g. generic rewrites of coll-of, map-of, etc (this issue)
• Shrinking the number of array elements shown when printing a value https://github.com/bhb/expound/blob/master/doc/arch/adr-003.md
• Hiding some extraneous parts of the printed value When keys spec fails, the entire map is printed out (not the summary form) #129
• Inject some additional content into the entire error message Idea: improve error messages with compile-time information of the faulty expression #148
• Remove headers or other non-optional parts of the message

My thinking is that explain-data is not really the right structure here, because it represents the problem, not the error message. Instead, I am imagining something analogous to a Hiccup data structure which represents the document but with semantic tags.

Right now, Expound is a function from explain-data -> string, but I can imagine there being an intermediate layer explain-data -> error-document-data-structure -> string.

As a result, for every non-common feature request, the user could just introduce a function that transforms the error-document-data-structure to another valid error-document-data-structure and use Expound to do the last step of error-document-data-structure -> string.

I believe this would side-step the need for set-msg-fn!, because you could just navigate to the part of the document you care about, then use meander to rewrite the form however you wish.

[mpenet]

Right now, Expound is a function from explain-data -> string, but I can imagine there being an intermediate layer explain-data -> error-document-data-structure -> string.

Yes, that would be the ideal solution indeed.

[bhb]

@mpenet Thanks for the suggestion and the code! After a lot of consideration, I decided to not include support for messages for predicates and arbitrary spec forms, since this complicated the API considerably.

Instead, if you want to reuse the same error message, you can declare your own specs.

(require '[expound.alpha :as expound])
  
(s/def :myapp/string string?)
(s/def :myapp/name :myapp/string)
  

(expound/defmsg :myapp/string "should be a string")

(expound/expound :myapp/name :sally {:print-specs? false})
;; -- Spec failed --------------------

;;   :sally

;; should be a string  

[mpenet]

Thanks for the notification. I understand your decision.

[bhb]

@mpenet Thanks for all your help here. I've just released 0.9.0, which includes this functionality.