add feature to provide an additional spec to encode/decode functions.

[wandersoncferreira]

The idea is to validate the target transformed value. The current
implementation will only make transformations that does not break the
provided spec as described in #241 .

While implementing this feature I noticed something odd, https://github.com/metosin/spec-tools/blob/master/src/spec_tools/core.cljc#L412, because of this decision I often get an ClassCastException value instead of a ::s/invalid because the wrong value is allowed to be parsed and the call to (s/unform spec value) in st/encode cannot make sense of it.

Unfortunately, seems like we cannot remove it from the code without breaking changes. I could not figure out what is the use case behind it. Maybe @ikitommi or @miikka can provide more context, if possible.

Also, this implementation introduces another dynamic binding and increase a bit the complexity of the conform* implementation. I can see the value added as described in the issue page and my avaliation is that would be good to have it encorporated despite of that.

Thanks!

[codecov-io]

Codecov Report

Merging #248 (3ef9480) into master (65bb923) will decrease coverage by 0.03%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage   88.03%   87.99%   -0.04%     
==========================================
  Files          16       16              
  Lines        2039     2049      +10     
  Branches      183      183              
==========================================
+ Hits         1795     1803       +8     
- Misses         61       63       +2     
  Partials      183      183              

Impacted Files	Coverage Δ
src/spec_tools/core.cljc	94.37% <88.88%> (-0.44%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65bb923...3ef9480. Read the comment docs.

[miikka]

Hello again and sorry for taking some time to get back to you! The code looks good and in my opinion, this is a desirable feature – I've wished for it a couple of times myself.

Unfortunately I don't really understand why the code is like that. If it was fixed, what would break?

[wandersoncferreira]

Hi @miikka no problem at all. Just to be clear before addressing the issue I mentioned, the problem with the exception being raised is not related to the implementation of this PR. It was something I step into at the beginning of the implementation while I was reproducing the example from #241

To make sure, I run the following example (extracted from core_tests and slightly modified) against master and these are the results:

(s/def ::my-spec
  (spec
    {:spec #(and (simple-keyword? %) (-> % name clojure.string/lower-case keyword (= %)))
     :description "a lowercase keyword, encoded in uppercase in string-mode"
     :decode/string #(-> %2 name clojure.string/lower-case keyword)
     :encode/string #(-> %2 name clojure.string/upper-case)}))

(s/def ::my-spec-map (s/keys :req [::my-spec]))

(let [invalid-but-does-work {::my-spec "KIKKA"}
      invalid-but-does-not-work {::my-spec {:testing "KIKKA"}}]
  (encode ::my-spec-map invalid-but-does-work string-transformer) ;; => #:spec-tools.core{:my-spec "KIKKA"}
  (s/valid? ::my-spec-map invalid-but-does-work) ;; => false
  (s/unform ::my-spec-map invalid-but-does-work) ;; => #:spec-tools.core{:my-spec "KIKKA"}
  

  (encode ::my-spec-map invalid-but-does-not-work string-transformer) ;; => 1. Unhandled java.lang.ClassCastException
  (s/valid? ::my-spec-map invalid-but-does-not-work) ;;=> false
  (s/unform ::my-spec-map invalid-but-does-not-work) ;; => #:spec-tools.core{:my-spec {:testing "KIKKA"}}
  )

I was not expecting the ClassCastException, it should be ::invalid or the correct result. I would like to investigate this problem in a different PR.

[miikka]

I was not expecting the ClassCastException, it should be ::invalid or the correct result. I would like to investigate this problem in a different PR.

Ok, I agree with regards to the result and another PR sounds good.

[miikka]

@ikitommi What do you think about this? I think we should merge it but would like to have your opinion, esp. w.r.t. adding a new dynamic binding.

[ikitommi]

I propose a small change: instead of binding 3 dynamic vars, let's have:

• a new record DynamicConforming etc with the three fields
• bind and read just that one

=> dynamic binding is really slow, both to read & write.

otherwise, looks great!

[wandersoncferreira]

@ikitommi and @miikka I pushed a new version changing the three dynamic values to a single dynamic record.

[ikitommi]

LGTM

[miikka]

Okay, let's have it.