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

Reloading namespace with defrule results in IllegalArgumentException #44

Closed
andrew-nguyen opened this issue Mar 2, 2014 · 6 comments
Closed

Comments

@andrew-nguyen
Copy link

Just starting to play around with clara-rules and tried the simple example contained in the readme and a bit of an expanded one (instead of printing as a consequence of a rule, I inject a response record.

Seems that if I reload just the namespace that contains a bunch of defrule's, I get an exception "The given query is invalid or not included in the rule base." If I do a reload-all, then it works as I would expect.

If I use the example as is, it runs the first time and if I reload just the namespace, the println's don't seem to be executed.

@hugoduncan
Copy link
Contributor

Seems to have something to do with the redefintion of defrecords. Moving the defrecords to a different namespace, allows recompilation of the defrules.

@rbrush
Copy link
Collaborator

rbrush commented Mar 4, 2014

Unfortunately I'm traveling for the next couple days, but plan on taking a
look at this as soon as I get back. I have noticed some confusing behavior
when redefining defrecords. I think this is because it creates new Class
instance that is not equal to the previous, which is retained by the rule.
We might be able to fix this by matching on qualified class name for the
record than the record instance. I'll give that a spin when I get back from
my trip.

In any case, thanks for reporting this!

@mrrodriguez
Copy link
Collaborator

Just a few thoughts (my 2 cents).

I think it debatable if someone should expect that a re-def'ed defrecord will still be considered
the same defrecord type as the rules were compiled against. This isn't really the typical semantics behind doing such a thing, since it is just a brand new Class under a new loader.

This sounds potentially to just be a REPL issue to me. I suppose it does sound interesting to think about if matching on something that doesn't change, like the qualified name, would cause much disruption though. Maybe it is more Clojure-like to just think of the type generated via defrecord as a :type key attribute of a Map, in which case a re-def shouldn't mean a different type.

I think the approach of keeping the defrecord's in a different Namespace, where they won't be continually (forced) to be reloaded, would be a common way of dealing with this sort of thing from what I've experienced.

@andrew-nguyen
Copy link
Author

Generally, I think I agree with Mike on this.  I actually haven't used defrecord much in my own code and where I have, I haven't created a situation that would highlight similar behavior.  Putting defrecords in a separate namespace seems to be a clean enough solution.

I do also like the idea where a re-def doesn't mean a different type altogether but will think about it a little more.

Thanks for a cool project and an interesting discussion.

@rbrush
Copy link
Collaborator

rbrush commented Mar 5, 2014

I tend to agree with Mike on this as well. Interestingly, Clojure protocols have similar problems: if a record is re-def'd, any protocol extensions of that record need to be reloaded as well to work with the new version. So I think it's fair for Clara to have similar constraints as Clojure's protocol constructs do.

I'd actually like to see this handled better with protocols -- if some facility came along to deal with that, we should follow suit here. But that's an entirely different discussion. :)

@rbrush
Copy link
Collaborator

rbrush commented May 11, 2014

I'm going to close this due to the above discussion, but feel free to re-open or create another issue if you disagree. Re-loading records isn't a great experience in Clojure in general so the issues described here seem like something that could be considered Clojure-wide rather than in this project.

@rbrush rbrush closed this as completed May 11, 2014
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

No branches or pull requests

4 participants