Added PathRule that evaluates it's composite rules if it's primary ru…

[dagframstad]

…le evaluates to true

[coveralls]

Coverage increased (+0.4%) to 91.225% when pulling 98bcb47 on dagframstad:PathRule into 31eee54 on j-easy:master.

[fmbenhassine]

Hi @dagframstad

Thank you for your PR! Even though it is excellent (production code + test code), I'm not sure if this feature should be provided by easy rules. It's always a tradeoff to decide what should go in the tool/framework and what should be left to the user. In this case, a "PathRule " is a specific requirement and can be left to the user when needed. Do you agree?

Kr
Mahmoud

[fmbenhassine]

Hi @dagframstad

Any update?

Kr
Mahmoud

[coveralls]

Coverage decreased (-2.8%) to 88.06% when pulling dcec995 on dagframstad:PathRule into 31eee54 on j-easy:master.

[coveralls]

Coverage decreased (-0.7%) to 90.112% when pulling a4213af on dagframstad:PathRule into 31eee54 on j-easy:master.

[dagframstad]

Sorry for late reply.

Regarding your earlier comment if this PathRule should be left to the user to implement.
I don't agree, I don't think the PathRule differs much (in concept) from your CompositeRule, and a bit of flexibility is always good.
But it's your project, so you decide. I won't get offended if you don't include it.

[fmbenhassine]

Hi @dagframstad ,

As commented here, I suggest to deprecate the CompositeRule and move it to another module. Since the semantics of composite rule may vary, I like to make it abstract and each one can define its logic.

In regards to your PathRule, as said it is a good idea. Just a couple of notes:

• Since it will be a subclass of CompositeRule, I suggest to consider the rule with highest priority as the primary rule. This is to make it consistent in construction with other composite rules. When we create a composite rule, we can add rules to it. With the current design, we have to make a distinction between the primary rule and other rules (addPrimaryRule(Rule) or addRule(Rule rule, boolean primary) or any other mean to make the distinction).

• I suggest another name, FlowRuleGroup also for consistency with other subclasses of CompositeRule. In the end, it is some kind of flow: if the primary rule is ok, then trigger the rest. I was also thinking of otherwise semantics (but this is not consistent with current design of having no else statement).

Anyway, let me know what do you think.

Kr
Mahmoud

[cemo]

Maybe @dagframstad wanted a condition and multiple action. What is the use case for PathRule?

[fmbenhassine]

@cemo I will let @dagframstad reply but from my understanding from the code, the idea is to have a primary rule that acts as a precondition to trigger a rule group. If the condition of the primary rule is satisfied, the primary rule's action is executed, then other (applicable) rules will be executed.

@dagframstad I suggested renaming this to FlowRuleGroup, another possibility is ConditionalRuleGroup which is probably better than FlowRuleGroup.

[dagframstad]

@benas Your understanding of my contribution here is correct, a single rule to decide if we should execute a group of rules.

ConditionalRuleGroup sounds like a better name than the one I came up with (I struggled to find a good name).

(There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.)

[fmbenhassine]

@dagframstad Great!! As explained here, I have started the preparatory work to welcome your PR.

Could you please update the PR as follows:

• Rename PathRule to ConditionalRuleGroup
• Make it a subclass of CompositeRule and move it to the easy-rules-support module (next to UnitRuleGroup)
• Change the logic to consider the rule with highest priority as the primary rule (as discussed previously, for consistency with other composite rules).

You can rebase your work on the unit-rule-group branch.

Many thanks upfront.
Kind regards
Mahmoud

[coveralls]

Coverage increased (+0.4%) to 89.756% when pulling 5aced14 on dagframstad:PathRule into f904f33 on j-easy:unit-rule-group.