kie-issues#237: Implement DMN 1.4 Boxed iterator expression#2243
kie-issues#237: Implement DMN 1.4 Boxed iterator expression#2243tiagobento merged 2 commits intoapache:mainfrom
Conversation
kie-issues#238: Implement DMN 1.4 every/some -> in -> satisfies boxed expression Co-authored-by: Tiago Bento <1584568+tiagobento@users.noreply.github.com>
danielzhe
requested review from
ederign,
paulovmr and
tiagobento
as code owners
tiagobento
left a comment
tiagobento
left a comment
There was a problem hiding this comment.
@danielzhe Thanks for the PR! I left some comments inline.
| return ret; | ||
| } else if (args.rowIndex === 2) { | ||
| if (prev.__$$element === "for") { | ||
| const ret: BoxedFor = { |
There was a problem hiding this comment.
Missing comment "// Do not inline this variable for type safety. See microsoft/TypeScript#241"
| } else { | ||
| // Do not inline this variable for type safety. See https://github.com/microsoft/TypeScript/issues/241 | ||
| const iterator: BoxedIterator = { | ||
| ...prev, | ||
| satisfies: { expression: undefined! }, // SPEC DISCREPANCY | ||
| }; | ||
| return iterator; | ||
| } |
There was a problem hiding this comment.
Defaulting to "every" and "some", while I think we should actively check for those types and throw an Error if anything else is found.
| return iterator; | ||
| } | ||
| } else { | ||
| throw new Error("ConditionalExpression shouldn't have more than 3 rows."); |
| else if (expression.__$$element === "for") { | ||
| const nestedExpressions = [expression.in.expression, expression.return.expression]; | ||
| return ( | ||
| resizingWidth ?? | ||
| CONDITIONAL_EXPRESSION_LABEL_COLUMN_WIDTH + | ||
| Math.max( | ||
| CONDITIONAL_EXPRESSION_CLAUSE_COLUMN_MIN_WIDTH, | ||
| ...nestedExpressions.map((e) => getExpressionResizingWidth(e, resizingWidths, widthsById)) | ||
| ) + | ||
| CONDITIONAL_EXPRESSION_EXTRA_WIDTH | ||
| ); | ||
| } | ||
|
|
||
| // Every and Some | ||
| else if (expression.__$$element === "every" || expression.__$$element === "some") { | ||
| const nestedExpressions = [expression.in.expression, expression.satisfies.expression]; | ||
| return ( | ||
| resizingWidth ?? | ||
| CONDITIONAL_EXPRESSION_LABEL_COLUMN_WIDTH + | ||
| Math.max( | ||
| CONDITIONAL_EXPRESSION_CLAUSE_COLUMN_MIN_WIDTH, | ||
| ...nestedExpressions.map((e) => getExpressionResizingWidth(e, resizingWidths, widthsById)) | ||
| ) + | ||
| CONDITIONAL_EXPRESSION_EXTRA_WIDTH | ||
| ); | ||
| } |
There was a problem hiding this comment.
Not using the ITERATOR_EXPRESSION constants.
| } else { | ||
| return ( | ||
| <IteratorExpressionCell | ||
| iteratorClause={props.data[props.rowIndex]} | ||
| {...props} | ||
| parentElementId={parentElementId} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Defaulting to this, while I think we should explicitly check for rowIndex being 1 and 2, as iterator expressions shouldn't have more than 3 rows.
| } else { | ||
| if (expression.__$$element === "for") { | ||
| return "return"; | ||
| } else return "satisfies"; |
There was a problem hiding this comment.
Same here. Defaulting to "satisfies", while we should check for explicit cases we support and throw an error for all other unexpected cases.
| const getIterableRowLabel = useCallback( | ||
| (rowNumber: number) => { | ||
| if (rowNumber === 0) { | ||
| return expression.__$$element; |
There was a problem hiding this comment.
Using __$$element here is not ideal, as it is a coincidence that those two things have the same value. Please explicitly return the name of the expression based on what type of iterator it is.
|
|
||
| const currentExpression = useMemo(() => { | ||
| if (typeof iteratorClause.child !== "string") { | ||
| return iteratorClause.child?.expression as any; |
| expression: getNewExpression(), | ||
| }, | ||
| }; | ||
| } else { | ||
| return { | ||
| ...prev, | ||
| satisfies: { | ||
| expression: getNewExpression(), |
There was a problem hiding this comment.
Why not pass prev here, like you're doing at the in part?
| return { | ||
| ...prev, | ||
| "@_iteratorVariable": updatedValue, | ||
| }; |
There was a problem hiding this comment.
Please follow the same strategy of extracting the return value to a variable, explicitly typing it and adding the comment saying why this is necessary.
ljmotta
left a comment
ljmotta
left a comment
There was a problem hiding this comment.
@danielzhe Amazing new features. The code is looking good.
|
Hi @ljmotta @tiagobento @danielzhe I've just tested this new functionality and I'm getting the following exception:
KIE.Sandbox.__.Untitled.dmn.-.Google.Chrome.2024-04-18.15-27-22.mp4Build Version: Google Chrome Version 123.0.6312.123 (Official Build) (64-bit) in Windows 10 |
|
@jaimeizuz Ty for your report. It got broken in the code review, I'm already fixing it. |
4 participants
Closes: apache/incubator-kie-issues#237
Closes: apache/incubator-kie-issues#238
Both tickets were made together because the
iterator expressioncovers both as it is in the spec. See 10.2.1.10 Boxed iterator