Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DMN 1.5 - 1155-list-replace-function #657

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

StrayAlien
Copy link
Contributor

@StrayAlien StrayAlien commented May 15, 2024

Suite of pretty standard tests for the new "list replace" function. Exercises good/bad positional and named params. Also negative positioning.

@baldimir
Copy link
Collaborator

@dmn-tck/contributors please review this.

@SimonRinguette
Copy link
Contributor

Regarding test 019: list replace ( [2, 4], function(item, newItem) item, 5). The function body item return a number and not a boolean. My personal interpretation is that the item will be replaced only if the return value is true. If its null/a number or otherwise, it will not replace it. From the spec: return a new list where newItem replaced at all positions where the match function returned true. However the spec also says ` boolean function(item, newItem).

image

My interpretation is that this test case should return [2,4], certainly not null.

Regarding test 011: list replace([1,2,3], 2.5, 4). This comes back to the discussion of what happens with the rounding for integer. The test case assumes this returns null. Based on the previous discussion of trimming, this test should return [1,4,3].

@StrayAlien
Copy link
Contributor Author

Thanks @SimonRinguette . Good comments.

Re list replace ( [2, 4], function(item, newItem) item, 5) returning null. My reasoning there is that the signature of the function as per the spec is to return a boolean ... and it does not. Thus the function is not spec compliant - and is an error condition. Happy to discuss for sure.

Regarding the rounding. To my mind the spec is pretty clear - it 'must' be an integer. No provision for automatic rounding is given. When the spec says something is a must, to my mind, it is a must. Also happy to discuss.

position must be a non-zero integer (0 scale number) in the range [-L..L], where L is the length of the list

@opatrascoiu
Copy link
Contributor

On test 019: list replace ( [2, 4], function(item, newItem) item, 5) I agree with @StrayAlien .

On test 011: list replace([1,2,3], 2.5, 4) I agree with @SimonRinguette. This issue was recently discussed in the RTF meeting, the final decision was to add a truncation from 2.5 to 2 to be more user-friendly and in synch with other programming languages. This approach was added in DMN 1.6 - DMN16-84. We also have similar tests see 1103-feel-substring-function

@baldimir
Copy link
Collaborator

Agreed about the case when the function doesn't return a boolean value - we agreed that it should be an error state and behave as @StrayAlien defined.

@StrayAlien
Copy link
Contributor Author

From TCK meeting. Note to @StrayAlien, Chang test to truncate to whole number.

@StrayAlien
Copy link
Contributor Author

tests has been updated to show both positive and negative decimals being truncated

Copy link
Contributor

@yesamer yesamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests were run against Apache KIE, and no issues were found.
Approved, thank you @StrayAlien

@opatrascoiu
Copy link
Contributor

@baldimir @StrayAlien Looks good to me - ready to merge. Thank you @StrayAlien

@baldimir
Copy link
Collaborator

@SimonRinguette is this ok for you to merge, please?

@SimonRinguette
Copy link
Contributor

All good, merging

@SimonRinguette SimonRinguette merged commit 2c14149 into dmn-tck:master Oct 18, 2024
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants