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

simple implementation of unwrapping dataspecs from specs created usin… #226

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wandersoncferreira
Copy link
Contributor

@wandersoncferreira wandersoncferreira commented Apr 6, 2020

Hello @ikitommi , I found a nice ticket open (#111 ) and I wanted to give it a shot. This is a simplified implementation of unwrapping specs created from data-specs. I used multimethods to dispatch to common spec operators, but the current solution is not exhaustive therefore might have some possibilities not covered, need to check this out.

Some drawbacks.

  1. The current solution is only working in clj basically because I can't get the function out of symbols in cljs (this situation again). I run the test in cljs and got the following error message for the simpler specs I wrote:
FAIL in (unspecing) (:)
simpler possible specs
expected: (= boolean? (ds/unspec (ds/spec {:name :spec-tools.data-spec-test/t1, :spec boolean?})))
  actual: (not (= #object[cljs$core$boolean_QMARK_] cljs.core/boolean?))

As seems like cljs compile the boolean? function to a js object and I get a Symbol back from resolve macro in cljs.

  1. There is also a minor drawback when clojure.specs are used as keys in the data-spec, for example:
(s/def ::age pos-int?)
(ds/spec {:name  ::test :spec ::age) ;; =>  #Spec{:form clojure.core/pos-int?, :type :long, :leaf? true, :name :spec-tools.data-spec/test} 

As we can see the ::age keyword was transformed to a full form as clojure.core/post-int? therefore when I use (ds/unspec (ds/spec {:name ::test :spec ::age})) ;;=> #function[clojure.core/pos-int?]

It works, but is not the same as recovering the full qualified keyword that defined the pos-int? in the first place.

TODO: I would like to present this rationale for the solution to see if is viable in the long run, as well as discuss the unspec arity [I don't see why not only receive the spec itself, idk what kind of parameters would be nice to have as well]. After, I need to implement, ds/opt and ds/req over map keys.

Thanks!

@coveralls
Copy link

coveralls commented Apr 6, 2020

Coverage Status

Coverage decreased (-0.05%) to 96.429% when pulling a7112ad on wandersoncferreira:unwrapping-data-specs into f0bd074 on metosin:master.

@wandersoncferreira
Copy link
Contributor Author

wandersoncferreira commented Apr 7, 2020

Could not lose the opportunity to write a property-based test for the data-spec->spec->data-spec round trip. I found lots of corner cases in my first commit. Now should make changes to this code with a base solution as starting point.

I still don't totally get how a specific portion of a spec will be converted. The current solution has lots of hacks that I can probably clean up, the fact of existing so many ways to check and or create a spec is already complicated to me, s/spec?, st/spec?, fn?, s/get-spec, create-spec, if none works, try parse/parse-spec rsrsrs maybe this is part of the problem you were talking about at the malli presentation or maybe I am just not familiar enough yet :)

2020-04-09 EDIT: From my side, this is now ready to merge. If the drawbacks mentioned above is fine for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants