Skip to content

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

Open
wandersoncferreira wants to merge 3 commits into
metosin:masterfrom
wandersoncferreira:unwrapping-data-specs
Open

wandersoncferreira wants to merge 3 commits into
metosin:masterfrom
wandersoncferreira:unwrapping-data-specs

Conversation

@wandersoncferreira

@wandersoncferreira wandersoncferreira commented Apr 6, 2020 •

Copy link
Copy Markdown
Contributor

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

coveralls commented Apr 6, 2020 •

Copy link
Copy Markdown

Coverage Status

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

@wandersoncferreira

wandersoncferreira commented Apr 7, 2020 •

Copy link
Copy Markdown
Contributor Author

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.

This branch has not been deployed

No deployments
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