-
Notifications
You must be signed in to change notification settings - Fork 110
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
Load collections of rules in namespaces #134
Comments
Hi William, This sounds reasonable. I can imagine instances where we want higher-level constructs that produce multiple rules. I'd lean toward options A, unless there is some other significant advantage to B. A seems a bit simpler to me. If you want to put together a pull request (with a unit test) I can merge it in. It's also possible for tools generating rules to pass them directly into mk-session. I included an example of this in this discussion: https://groups.google.com/d/msg/clara-rules/JYEgfkns160/3zFDVFq7FAAJ I think this can satisfy several rule-generating use cases. However, the change proposed on this issue makes sense since I can still imagine where it would be nice to have multiple rules in a var. -Ryan |
Yes, I know mk-session can take rule structures directly (which is nice). In this case |
Closed when I was trying to comment back.. anyway: A is fine with me. Yes, I know you can give rule structures to mk-session directly (which is good). The motivation here is basically to have defrules and rules generated with our DSL fit together in an unsurprising way. I'll work on a PR for this. |
Makes sense. Thanks for working this! |
Just an FYI, I'm probably going to push out a 0.9 release of Clara in the next couple days. I can hold it up a bit to include this, or we can easily do a 0.9.1 release with this (and anything else) when needed. Either way is fine with me...I just wanted to avoid the possibility of pushing a release if a change was almost ready to include. |
I have created a pull request at #139 for which Travis CI passed. Note that this only adds the functionality for Clojure though, not ClojureScript. I see that the ClojureScript implementation uses a different mechanism for finding rules; it stores global state when defrule or defquery expands via the add-production function. It looks like ClojureScript supports compile-time metadata on vars as of the resolution of http://dev.clojure.org/jira/browse/CLJS-1046
Is there another reason that the add-production path is need, or was that written before the ClojureScript functionality was added? Obviously another add-production type thing could be added if we want this in ClojureScript, but it seems messier than metadata. For my immediate use-case I really only need Clojure, but consistency might be desirable. |
Looks great! I think it's fine to leave it out for ClojureScript, pending some refactoring of that use that I'm now tracking in issue #140. At the time this ClojureScript logic was written, it was not possible to enumerate or inspect vars, as they were removed as an optimization for the compiler. It appears this is no longer the case. Changing that is outside the scope of this issue, so I created #140 to follow up on it. I'm going to go over the code one more time and merge it unless I find something unexpected.0 |
Alright, merged and targeted for 0.9.0, which I will push out soon. Thanks for the very nice contribution! I'm closing this since I don't think there is anything more to do here, but we can reopen or create a new issue if you disagree. |
@WilliamParker do you a link to an example of |
@theronic Here is an example for Clojure and an example for ClojureScript. The reason for the difference is that ClojureScript sessions have to be created at compile time due to ClojureScript's lack of eval. You can still create sessions from arbitrary rules in both Clojure and ClojureScript, but for ClojureScript the decision of what rules to include has to be made at compile time. The relevant parts of the code for understanding the passing of either rule sources (such as a namespace symbol) or individual rules are where the compiler loads rules/queries from IRuleSource implementations or just considers them raw rules/queries, the IRuleSource protocol definition, and the IRuleSource implementation for Symbol that retrieves the rules from a namespace. It isn't necessary to understand how the rest of the compiler works to understand how rule sources work. That said, this isn't actually related to this particular GitHub issue. Prior to this issue, when namespaces were scanned for rules, Clara expected there to be one rule per var. This enhancement added (for Clojure only) the option to make vars containing sequences of rules be picked up as rules in a namespace when the var in question is given appropriate metadata. The ability to do this is useful for macros that generate rules that should be associated with a namespace. |
Currently the implementation of IRuleSource supports scanning namespaces for single rule or query definitions with :rule or :query on their metadata, respectively. [1] This works for the use case of having 1 production per Var. However, we are building internal tooling which will build seqs of rules. We could use macros to generate a var per rule, but this has the undesirable impact of creating lots of mappings in an ns that would be problematic in a REPL. My thought is that it might be helpful to enhance the implementation to also look for collections of productions (rule or query) structures in a namespace. This could be accomplished by adding another keyword to indicate this on Var metadata. Two options I see are:
A. Use metadata like {:production-seq true} to indicate that the var holds a seq of production structures. In this case the code would look something like
B. Store a function in the metadata that takes the value of the var and transforms it into production structures. This would look something like
In the simple case of the var holding a seq the function would just be identity.
Overall, this seems consistent with maintaining a pattern of rules-as-data to me. Obviously users can create their own loader implementations and wrap mk-session, but this seems cleaner, broadly useful, and would be passive to existing usage. If either of these enhancements are desired I can submit a pull request with tests.
The text was updated successfully, but these errors were encountered: