oracle-samples / clara-rules

Forward-chaining rules in Clojure(Script)
http://www.clara-rules.org
Apache License 2.0
1.2k stars 115 forks source link

Use clojure.spec.alpha #403

Open alex-dixon opened 6 years ago

alex-dixon commented 6 years ago

This has been mentioned elsewhere but doesn't seem to have a dedicated issue.

Continuing from @rbrush's comment:

Yes, I agree that we should include this in Clara proper and remove the old Prismatic schema. [...] we need to get on the evolutionary trunk of this stuff in Clojure. https://github.com/cerner/clara-rules/issues/367#issuecomment-385561169

1.10 alphas are out now. I have some specs in various places. I'm going to start gathering them into a branch with this issue number.

I could use some help on maintaining support for 1.7 and 1.8.

@cfleming on this:

this seems to be easy. As an example it's what clojure.java.jdbc does. In fact, the specs could even be kept in a totally separate artifact if required. https://github.com/cerner/clara-rules/issues/367#issuecomment-353488049

It looks like they only define specs in a single namespace and that this namespace is only required by a test: https://github.com/clojure/java.jdbc/blob/d8a35b9531b76a772467ca24b2d866b7bc29b6c6/src/test/clojure/clojure/java/jdbc_test.clj#L29 They use s/fdefs, so when the test is run with > 1.9 they turn on instrumentation, which throws spec errors for function vars the s/fdefs are defined for.

I don't know whether or how users get these errors, since the namespace with the fdefs doesn't appear to be required by anything other than this test.

This may be minor, but they don't require spec in any of their namespaces either. Without it, you can't use s/valid, s/assert, s/conform, or s/def inline with source code, which is a more common pattern than having separate spec-only namespaces, and provides more flexibility than s/fdef alone.

So potentially we could use s/fdefs only and conditionally require the namespaces with them based on *clojure-version*.

Thoughts?

metacritical commented 6 years ago

This parser would need to be written in parts for each spec type and slowly replaced. Is there a road map/steps to implement this requirement.

WilliamParker commented 6 years ago

@alex-dixon

So potentially we could use s/fdefs only and conditionally require the namespaces with them based on *clojure-version*.

That sounds reasonable - would it be better to detect the spec namespaces on the classpath themselves though, or perhaps the combination of the two?

@metacritical

This parser would need to be written in parts for each spec type and slowly replaced. Is there a road map/steps to implement this requirement.

Is this proposal to replace the existing logic in the DSL namespace with something from spec? https://github.com/cerner/clara-rules/blob/0.19.0/src/main/clojure/clara/rules/dsl.clj
To be honest, I've explored it a bit, but I haven't dug deeply into spec's APIs yet, just haven't had the time, and applications to Clara's parsing isn't something I've thought about. Do you have thoughts on how this could be done and what the benefits would be? I think the answer to "is there a road map" is that there isn't one, as far as I'm aware.

metacritical commented 6 years ago

@WilliamParker A benefit of 'clojure.spec' would be destructing of data is automatic based on the shape of data. So you can write your walker/parser code using a multi method dispatch, which is quite readable and thus maintanable, compared to case condition where we trying to read symbols, keywords and destructuring manually.

alex-dixon commented 6 years ago

you can write your walker/parser code using a multi method dispatch, which is quite readable and thus maintanable, compared to case condition where we trying to read symbols, keywords and destructuring manually.

The second way might be better if you’re relying on s/conform to parse syntax for you that may have specs registered for it. spec ends up conforming everything recursively, so it wouldn’t necessarily just conform using Clara’s defined specs — could be any registered spec. I’m not sure whether this could break existing users. May depend on when user defined specs are loaded relative to when Clara’s are.