Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limitation on using conformed values limits usefulness of some s/and specs #102

Closed
bhb opened this issue Jun 17, 2018 · 8 comments
Closed
Labels

Comments

@bhb
Copy link
Owner

bhb commented Jun 17, 2018

Expound doesn't support using conformers to coerce values, because it significantly complicates the way Expound highlights the bad value (Expound uses a bunch of heurisitics to try to disambiguate the information returned by explain-data and these heuristics rely on the original unconformed value appearing the larger structure).

However, this restriction means that Expound can't print error messages for some and specs, since and conforms the values.

user=> (require '[clojure.spec.alpha :as s])
nil
user=> (require '[expound.alpha :as expound])
nil
user=> (s/def ::sorted-pair (s/and (s/cat :x int? :y int?) #(< (-> % :x) (-> % :y))))
:user/sorted-pair
user=> (s/explain ::sorted-pair [1 0])
nil
val: {:x 1, :y 0} fails spec: :user/sorted-pair predicate: (< (-> % :x) (-> % :y))
user=> (expound/expound ::sorted-pair [1 0])
ExceptionInfo Cannot convert path. This can be caused by using conformers to transform values, which is not supported in Expound  clojure.core/ex-info (core.clj:4739)
@bhb bhb added the bug label Jun 17, 2018
@jmlsf
Copy link

jmlsf commented Jun 23, 2018

Sadly, it means expound fails on the very first example of instrumentation in the official spec guide use s/def (the ranged-rand example). I wonder, would it be possible to drop back to the original explain in cases where expound fails? That'd be better than just getting an exception.

@bhb
Copy link
Owner Author

bhb commented Jun 23, 2018

Ah, good point, that's not a great experience for someone working through the official docs 😞 . Thanks for pointing that out 😄

Certainly defaulting to normal output is one option I'm considering. I'm not sure if falling back on normal explain should be the default or something you can opt into with a configuration. Before I realized that and had this issue, I was leaning towards "you must opt in" since using specs for coercion was an advanced use case and not recommended for various reasons.

But given that this more common (and, as you point out, included in the official docs), it might make sense to make falling back to normal explain the default. Or it might be possible to print a reasonable error in this case by looking for the the conformed value.

In any case, the workaround for now is to wrap the printer in a try/catch and default to s/explain-printer e.g.

(require '[expound.alpha :as expound])

(defn safe-printer [explain-data]
  (let [exp (expound/custom-printer {})]
    (try
      (exp explain-data)
      (catch Exception e e
             (s/explain-printer explain-data)))))

(set! s/*explain-out* safe-printer)

(s/def ::sorted-pair (s/and (s/cat :x int? :y int?) #(< (-> % :x) (-> % :y))))

;; Usually use expound
(s/explain int? "a")
;;-- Spec failed --------------------
;;
;;  "a"
;;
;;should satisfy
;;
;;  int?
;;
;;-------------------------
;;Detected 1 error

;; If there is an issue, fallback on s/explain-printer
(s/explain ::sorted-pair [0 0])
;; val: {:x 0, :y 0} fails spec: :expound.alpha/sorted-pair predicate: (< (-> % :x) (-> % :y))

@bhb
Copy link
Owner Author

bhb commented Jun 25, 2018

I wonder if I could look for both the conformed and unconformed value in the original data, then show both. Something like:

[1 2]

when conformed to

{:x 1 :y 2}

should satisfy

#(< (-> % :x) (-> % :y))

I this more about this.

@carocad
Copy link

carocad commented Jun 25, 2018

@bhb I think you can rely on both conform and unform working for a "pure" and implementation since those are standard transformations: https://github.com/clojure/spec.alpha/blob/739c1af56dae621aedf1bb282025a0d676eff713/src/main/clojure/clojure/spec/alpha.clj#L1136

However, it is of course possible that a user just mixed conformers directly with an s/and call; I am guilty of this btw 😅.

Generally I dont create an unform function for those cases but just put identity to "get the job done". You would have a hard time getting back the values unformed.

So I guess that you would need some kind of heuristics to figure out if you are able to unform the values or if you just default to the classical output. Btw, if you default to the classical output, would it be possible to still have a warning printed before it mentioning that you are getting the default output due to the unform part? otherwise the user would be left wondering if expound is actually working or not.

Hope it helps

@bhb
Copy link
Owner Author

bhb commented Oct 26, 2018

@carocad @jmlsf I've got a fix for this issue in 0.7.2-SNAPSHOT. If you have time to try it out, please let me know what you think!

@carocad
Copy link

carocad commented Oct 26, 2018

@bhb first of all, I have to say "Thanks a lot for putting the effort of figuring this out"

I tried the following cases and they all worked wonderfully.

I think there are some cases where the output could be slightly more accurate but those are pretty much nice to have; here I am referring to the first case 'string parsing', where it is clear from the output that there is a single value not a collection so the text part of the value doesn't really fit. Unfortunately I have stopped using conformer since they were creating too many problems so I cannot test my previous errors anymore 😅

screen shot 2018-10-26 at 12 11 40

@bhb
Copy link
Owner Author

bhb commented Oct 27, 2018

@carocad Thanks very much for trying it out! I appreciate it.

I think there are some cases where the output could be slightly more accurate but those are pretty much nice to have

Yes, you're right: Expound could do more here, but since string parsing is advanced at best, and discouraged at worst, I'm not inclined to add much more here. I may change my mind in the future, but I think this is sufficient for now 😄

@bhb
Copy link
Owner Author

bhb commented Dec 13, 2018

Fixed in 0.7.2-SNAPSHOT (to be released soon)

@bhb bhb closed this as completed Dec 13, 2018
WhittlesJr added a commit to WhittlesJr/kaocha that referenced this issue May 31, 2019
WhittlesJr added a commit to WhittlesJr/kaocha that referenced this issue Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants