Rabobank-Archive / rule-engine

A rule engine supporting forward chaining
MIT License
47 stars 5 forks source link

Macros enabling removal of explicit Fact naming #45

Closed jhkuperus closed 8 years ago

jhkuperus commented 8 years ago

Previously, Facts had to be declared and named manually to guarantee uniqueness of names inside the engine:

object MyGlossary extends Glossary {
  val MyFact = new SingularFact[Int]("MyFact")
}

The redundancy of the Fact's name is not just a nuisance to type, it actually causes trouble when renaming the Fact, or trying to copy its declaration. In both situations, you must also alter the name parameter manually. The compiler and IDE tooling cannot help you track these errors down and so you are bound to find out about them at runtime, with potentially uninformative error messages.

This pull request adds several macros to the project which allow for much easier declaring of Facts. The macros run during compile-time and transform their invocation into the line of code above, extracting the name of the val and providing it as the name of the Fact being constructed.

Using these macros, the above example could be rewritten as follows:

import org.scalarules.utils.FactMacros._

object MyGlossary extends Glossary {
  val MyFact = defineFact[Int]
}

There are alternatives of the macro for creating SingularFact and ListFact instances, as well as variations accepting a value for the description parameter of the base fact.

In light of this added feature, this PR also includes a bump in the minor version of the project. Backwards compatibility is guaranteed because the old method of defining Facts manually is still available and unaltered.

jhkuperus commented 8 years ago

I lied a bit about the backwards compatibility. I replaced the body of the Glossary class and anyone currently using it will be facing compiler errors. I'm just about to push a commit restoring it to its former glory and moving the new code to a new class called MacroGlossary.

jqno commented 8 years ago

I think this is extremely cool stuff.

I'm not sure about your intent though, judging from your story. Do you mean to have the macro implementation replace the old implementation? Or do you always want to have the old one as a fallback?

I would assume the former, because of the issues you mention with renaming them etc.

But it feels like the macro implementation is still kind of a second class citizen. I notice you did deprecate the Glossary class, but not its methods. (Which means you can hide the warning in the imports, not in the code actually using it.) Also, the new glossary is called MacroGlossary, which refers to its implementation details and therefore feels uncomfortable as a general purpose solution. Are you worried that there are still issues with the macro implementation?

In other words: why not just throw out the old implementation and replace it with the macro stuff? Even in semantic versioning, you're allowed to have breaking changes when the major version is still 0 (which it is). I'm not sure how large your client code base has become, but it doesn't seem like such a big deal to just change everything to the macro implementations there -- or am I missing something here?

But: extremely cool stuff!

jhkuperus commented 8 years ago

As I stated out-of-bound: here I am trying to actually be backwards-compatible and you encourage me not to ;)

Anyways, you certainly have a fair point. Our user-base is probably still just 1 project, which is also in our own control. And actually the 'old way' is easy enough for someone to recreate if they're too scared of macros.

All in favour of removing the old way without deprecation and provide the macro-way as our pre-1.0 default, please +1 this particular comment. ( @jqno @vzorge @NRBPerdijk ) (Edit: 3+ votes make it so, just to be explicit) :) (Edit 2: Didn't await vote number 3) ;)

vzorge commented 8 years ago

Because of these changes, the value false is now always given for the isResult parameter for a Fact. We should probably remove this parameter for now. Its intended use was to differentiate between types of Facts (inputs, partial results and outputs). This was always a sub-optimal solution and it would be better to redesign this from scratch

jhkuperus commented 8 years ago

Yes, I agree. I'll remove it, since it has no value anymore whatsoever.

As a replacement, I was thinking about allowing users to define their own 'metadata' to associate with Facts, but I haven't worked out a design for it yet.

vzorge commented 8 years ago

What is keeping this pull request from being merged?

jhkuperus commented 8 years ago

Me giving @NRBPerdijk a chance to take a look at it first :)

We'll continue working on calculations again soon, I'll make sure it's merged before such time.

NRBPerdijk commented 8 years ago

Reviewed, looks rather nice! Macro's sure are different... :)

jhkuperus commented 8 years ago

Closes #11