Condition classes #356
Condition classes #356
Conversation
|
Will look at this today |
There was a problem hiding this comment.
@chris-pardy thanks, the state of the Condition class has been bothering me for a while, so if nothing else this is a nice refactor. Look forward to seeing your longer term ideas come to fruition.
I've got a lot going on in my personal life atm, so I didn't go over this with a fine comb; maybe a good idea to have someone else from your org give it a review.
| AllCondition, | ||
| AnyCondition, | ||
| ConditionConstructor, | ||
| neverCondition, |
There was a problem hiding this comment.
nit: Why's the casing different here?
There was a problem hiding this comment.
neverCondition is a constant the others are classes. more a style question then anything else.
Sounds good. I'll get a second set of eyes on it. If you think the approach at a high level is in the right direction I'll fix the build issues. |
This brings getting the priority of a fact into line with getting the value of a fact.
chris-pardy
force-pushed
the
condition-classes
branch
from
fd1a25f to
1b8f1aa
Compare
Split conditions into classes that can be independently evaluated. This will allow new condition classes to be added by changing the implementation of the condition constructor.
Support passing a condition constructor into the engine and the rules in order to allow for custom conditions.
Move the enforcement of top-level conditions in the rules and the engine to the top-level condition constructor this also allows for passing Condition instances to the setCondition method by default Ultimately this adds a huge amount of support for new types of conditions that can be plugged into the system.
chris-pardy
force-pushed
the
condition-classes
branch
from
1b8f1aa to
61ac8d5
Compare
|
@CacheControl After doing a bit of thinking I'm actually going to close this PR. Going into a 7.0 version I think this is the right approach but it's also changes pretty widely what can be done with something like the rule result. For instance if someone added a new "ternary operator condition" to the set of conditions then the structure of the rule result would need to account for this. |
|
Makes sense, thanks for doing due diligence before proceeding! |
A major refactoring of the code around rules / conditions that makes no change to the actual execution of the code but does have a large impact on the potential to extend the rules engine.
Previously the
Conditionclass was used for all the different levels of nested conditions. However only the comparison conditions could be executed with theall,any,not, andconditiontypes being handled by theRuleclass. In an effort to make this more extensible I've split the code into 5 condition subclasses for each type of condition and moved the handling code into the condition, out of theRule. Additionally rather than theConditionconstructor knowing how to build a condition I've added a newConditionConstructorclass that knows how to build conditions and passes itself to theConditionsubclasses.With the
ConditionConstructorpresent I've added support for passing it to the Rule and Engine class and then moved the check for top-level conditions to aTopLevelConditionConstructorclass. By default there is no change to behavior of the engine, however you can easily set the conditionConstructor property on the Engine to do things like drop the need for top-level conditions, or add new types of conditions. New types of conditions in particular allow for vastly new features to be added that are effectively impossible to support now.