ISSUE-362: Refactor defrule and defquery for ClojureScript to better support...

[sparkofreason]

...customization of rule/query DSL.

[WilliamParker]

@sparkofreason just wanted to let you know that I've seen this PR and will review it. I like the idea, I just haven't had time to review it recently.

[sparkofreason]

@WilliamParker thanks, no problem. One thing worth noting is that the current solution only applies to CLJS, and I think parity in CLJ should probably also be a goal.

[WilliamParker]

The idea here looks sound to me @sparkofreason . As you suggest though it would probably be better to have parity between Clojure and ClojureScript here. My suggestion would be to add the build-rule and build-query functions to the dsl namespace, and then call them in both clara.rules.macros (for ClojureScript) and clara.rules (for Clojure). The signatures would need to change slightly to account for the (meta &form) argument that are being passed in to parse-rule* and parse-query* in Clojure. If ClojureScript has the same available it could do likewise; the usage appears to be in giving more informative exceptions from throw-dsl-ex.

Fundamentally, the build-rule and build-query functions seem like DSL logic to me, and putting them there would allow sharing between clara.rules, clara.rules.macros, and other users in both Clojure and ClojureScript macros. It would also reduce our codebase's size a bit and enforce more similarity between Clojure and ClojureScript.

Thoughts/objections/alternative suggestions?

[sparkofreason]

I'll dig into this tomorrow, just got back from holiday travels. That all sounds fine, just note that once we move those functions to a different namespace we won't be able to use the CLJS env/*compiler* any more for storing productions during CLJS compilation. I tested on my local copy with an atom defined in the DSL namespace, and everything seems to work fine. I can't think of any reason why env/*compiler* is required for this case. It's just an atom, with the added feature that the CLJS compiler resets it for each namespace being compiled, but that isn't being used here AFAICT.

[sparkofreason]

Also, I think I mentioned this in the discussion of #362: as part of improving support for 3rd-party DSLs, I'd like to support keyword-named rules and queries. Let me know if that's okay to include here, or if I should make a separate issue/PR.

[WilliamParker]

@sparkofreason I can't speak for anyone else, but I was thinking of having build-rule be in the dsl namespace, not the defrule! function. I think two different implementations of the latter in the macros and rule namespaces for clj and cljs respectively seems reasonable. Even if they were the same function you'd end up with two different implementations controlled by a switch between clj and cljs, just in the same fn body. I don't think build-rule has the same concerns though unless I'm missing something?

Regarding keyword names, so you're proposing something like

(defrule! :your-name-here rule-body-here)

That seems like something we could discuss, but I'd suggest logging another issue for it so that it can be discussed in a more visible way than deep in the comments of another issue.

[sparkofreason]

Re: defrule! and build-rule namespaces, seems reasonable.

For keyword support, I was thinking of just adding the support internally so other DSLs could use it, rather than modifying the default clara DSL. The former is a pretty minor change change, just checking if the name is a keyword and transforming appropriately (see Provisdom@6dd773b). The latter seems like a bigger deal with no obvious benefit, and feels like it breaks the var/namespace abstraction in the current DSL.

[sparkofreason]

See #371.

[WilliamParker]

I made a few minor comments but basically the change looks good.

[WilliamParker]

Minor: it might be a little easier to read if we reduced the duplication in forming the name by factoring out the ns construction logic into a let binding and sharing the rest. Something along the lines of:

name (assoc :name (let [query-ns (clojure.core/name (if (com/compiling-cljs?) (com/cljs-ns) (ns-name *ns*)))]
                               (str query-ns "/" (clojure.core/name name)))

[sparkofreason]

Done.

[WilliamParker]

Same as the comment on building the name in build-query

[WilliamParker]

Issue 359 is in the changelog already on line 5. I believe I changed the changelog message when I did the release on that particular entry; basically I just wanted to specifically indicate what the issue was rather than just indicating that an issue existed. If you think it could be more clear I'm open to making changes, we'll want to have a single bullet point in the changelog though.

[sparkofreason]

I just messed up the merge from upstream. Removing.

[WilliamParker]

Looking back at this I'm not sure why we don't add a docstring to the var def the same way as in Clojure. That's the status quo though, we don't need to make changes in this PR.

[WilliamParker]

I changed a "query-ns" local let binding in the build-rule function to "rule-ns" using the GitHub web editor; I presume that was just a copy-paste error from build-query. Travis will verify that that compiles/builds properly.

This looks good to me now, setting aside the issues in #371. Since this one looks ready I suggest going and merging this; then I need to take a look at the issues in #371 when I get a chance.

Does anyone else want to look this over before merging?

[alex-dixon]

LGTM.

Maybe we should add an issue to document how it can/should be used?

Didn't notice this before but looks like parse-rule* depends on the binding of ns. build-rule does also (because it calls parse-rule*) but also here: https://github.com/Provisdom/clara-rules/blob/d1c903dde46b9b3f364af4c66ca2116936d2b3c9/src/main/clojure/clara/rules/dsl.clj#L307

I don't know any ramifications to any of that...maybe we could consider mentioning it in the docstring like we have for parse-rule*?

Should there be an optional ns arg that defaults to *ns*? Not strictly necessary in CLJ since you can always call with (binding [*ns* my-ns] ...), but in CLJS doesn't seem like we give the user the option over what namespace the rule is defined in.

[sparkofreason]

I'm working on a project (https://github.com/Provisdom/maali) which uses a custom DSL, and would be happy to contribute some documentation for DSL-builders based on that.

[WilliamParker]

@alex-dixon actually I think that the ns can be controlled in ClojureScript; see this code qualifying symbols. Basically it is just another dynamic variable to bind, namely *cljs-ns*. I'd be open to discussing allowing the user to pass in an explicit argument, but IMO we can do that in another issue, noting that we haven't released these changes yet even if merged. I do think we'd want to have the ns of the rule name and the logical namespace of the rule (i.e. the ns where the forms are evaluated) to be the same if the name corresponds to a var in a namespace. Technically this doesn't need to be the case, but it seems confusing if it isn't. This could related to #371 as well since that would change the expectations around production names.

@alex-dixon @sparkofreason Adding some documentation for this sounds like a fine idea. :) Perhaps an additional section under "Advanced Topics" at clara-rules.org? The source for the site is at https://github.com/cerner/clara-site in case you didn't know. Feel free to suggest improvements, submit PRs, etc. to that project as well, for this or other topics.

I'm going to merge this since it has been in its current state for a while without any objections to doing so. The further issues on this seem like things we can iterate on after a merge.