[incubator-kie-issues#1543] Add the "id" of executed rules to the AfterEvaluateDecisionTableEvent #6127
Conversation
…erEvaluateDecisionTableEvent
| @@ -153,7 +153,7 @@ protected List<List<UnaryTest>> objectToUnaryTestList(EvaluationContext ctx, Lis | |||
| */ | |||
| private static DTDecisionRule toDecisionRule(EvaluationContext mainCtx, FEEL embeddedFEEL, int index, List<?> rule, int inputSize) { | |||
| // TODO should be check indeed block of inputSize n inputs, followed by block of outputs. | |||
| DTDecisionRule dr = new DTDecisionRule( index ); | |||
| DTDecisionRule dr = new DTDecisionRule( index, null ); | |||
There was a problem hiding this comment.
@gitgabrio If is legal to have a null id, why don't have a specific constructor with the index only?
There was a problem hiding this comment.
good point....
Well, this specific case seems sort-of corner case, since DecisionTableFunction has been removed (IINW) long time ago and we keep it for compatibility.
So, adding another constructor to DTDecisionRule would make it a little bit "ambiguous" just to cope for an exceptional case. From now on, DTDecisionRule should always have an "id" - and if the model does not define it, the id would be null- but it is slightly different then allowing instantiation completely without it: am I clear ? Does this make sense ?
| * | ||
| * <p> | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * <p> |
| * | ||
| * <p> | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * <p> |
There was a problem hiding this comment.
Thank you for a PR @gitgabrio , I have single comment to the code, optional for sure.
| decisionTable.getName(), | ||
| matchedIndexes ); | ||
| } | ||
| List<DTDecisionRule> matchIndexes = items.matches(); |
There was a problem hiding this comment.
When I compare this with a very similar logic in DecisionTableImpl.java I would maybe stick also here to the variable name matches index of matchIndexes as DRDecisionRule contains more data than index.
There was a problem hiding this comment.
HI @jomarko I agree and see your point.
Anyway all that part needs review, i.e.
- verify if that alphanetbased part is actually used anywhere
- double check the usage of that
Indexedinterface (from which the namematchIndexescame originally) - verify/remove all code-duplication, eventually
|
kogito-runtimes failure unrelated serverless-workflow-examples failure unrelated |
…erEvaluateDecisionTableEvent (apache#6127) * [incubator-kie-issues#1543] Add the "id" of executed rules to the AfterEvaluateDecisionTableEvent * [incubator-kie-issues#1543] Fix TODO * [incubator-kie-issues#1543] Fix license header * [incubator-kie-issues#1543] Minor refactoring on unrelated test * [incubator-kie-issues#1543] Fix as per PR review --------- Co-authored-by: Gabriele-Cardosi <[email protected]>
Fixes apache/incubator-kie-issues#1543
How to retest this PR or trigger a specific build:
for pull request and downstream checks
for a full downstream build
run_fdbfor Jenkins PR check only
Build Nowbutton.