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

mk-session docstring: document sources #477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

holyjak
Copy link

@holyjak holyjak commented Mar 15, 2023

It wasn't clear from the docstrings that mk-session actually takes rule sources, and in what form. It's better to include the information from http://www.clara-rules.org/docs/ruleforms/ right here.

It wasn't clear from the docstrings that mk-session actually takes rule sources, and in what form. It's better to include the information from http://www.clara-rules.org/docs/ruleforms/ right here.
Copy link
Contributor

@EthanEChristian EthanEChristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks holyjak, the contract on this macro is a bit fiddly and clarification was certainly needed.

If no sources are provided, it will attempt to load rules from the caller's namespace,
which is determined by reading Clojure's *ns* var.

`sources-and-args` can start with 0+ sources, each being either a namespace symbol or
Copy link
Collaborator

@mrrodriguez mrrodriguez Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an improvement to the doc string overall.

I do think it's a bit more strict in the doc string than the actual fn is though.

The concept of a "rule source" is protocol-based. The caller could give other data types that represent rule sources.

There is a clara.rules.compiler/IRuleSource protocol that has a few built-in extensions to it for things like clojure.lang.Symbol as can be seen here. People can extend it to other things, eg. clojure.lang.Var types if they wanted to (I've done this before). If what is given does not satisfy this protocol, then it is assumed to indeed be a sequence of rule/query (aka "production") data structures. This logic can be observed here.

Copy link
Collaborator

@WilliamParker WilliamParker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mrrodriguez 's comments on IRuleSource implementors being valid alternative arguments and also that this is a net improvement on the documentation. I also agree with renaming "args" to the more descriptive "sources-and-args".

@holyjak if you want to add that here, feel free, but otherwise I'm inclined to merge this soon unless there are objections. I'm happy to follow up with a short note about ISourceRule in the docs afterward. Thanks for pointing out the missing info!

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.

None yet

4 participants