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

Issue237: Add type to AlphaNodes for introspection #450

Merged
merged 2 commits into from
Feb 11, 2020

Conversation

EthanEChristian
Copy link
Contributor

A quick addition for #237

@mrrodriguez @WilliamParker

@@ -524,7 +524,7 @@
;; Record representing alpha nodes in the Rete network,
;; each of which evaluates a single condition and
;; propagates matches to its children.
(defrecord AlphaNode [id env children activation]
(defrecord AlphaNode [id env children activation type]
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the id part of #237 has already been done?

Also, should it perhaps be fact-type to be clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, and i have updated it to be fact-type

Copy link

@MrMikeRodriguez MrMikeRodriguez left a comment

Choose a reason for hiding this comment

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

wrong account

Copy link
Collaborator

@mrrodriguez mrrodriguez left a comment

Choose a reason for hiding this comment

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

So final question is does this work ok with the durability layer?

I believe we would be relying on the value of fact-type to be able to be serializable (via the fressian impl currently). I think this is on the rulebase too. Can this value be of any arbitrary type or is it restricted to things like symbols (naming a class), keywords, or other Clojure literal-expressed data structures?
ie. do different :fact-type-fn's open up the door for this to not be something that can be serialized with the built-in rulebase durability impl?

@EthanEChristian
Copy link
Contributor Author

@mrrodriguez,
For the Durability question, I would expect that any type provided would already have to be serializable as we have to serialize the alpha-roots, ie.

{fact-type [AlphaNode]}

It wouldn't be unreasonable to believe that a user could provide a fact-type-fn that produced something that wasn't serializable, but that would have tanked durability before these changes as well. Additionally, i would think that based on the contract(fressian-handlers), that in the event that a fact-type-fn like this was provided then the user should also provide the appropriate handlers.

@mrrodriguez
Copy link
Collaborator

#450 (comment)

@EthanEChristian that makes sense to me. I wasn't positive we were already serializing this same value - but indeed that should be the same thing.

@mrrodriguez
Copy link
Collaborator

When this is done, does #237 need closed?

@EthanEChristian
Copy link
Contributor Author

Yes, once this goes in i was going to close it.

I was trying to knock this out in an attempt to get better generated function names for #261 and #291

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.

LGTM; agreed with your analysis of the issues around rulebase serialization @EthanEChristian .

@EthanEChristian
Copy link
Contributor Author

Alright, I am going to merge this and get #237 closed out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants