dmn-tck / tck

Decision Model and Notation Technology Compatibility Kit
https://dmn-tck.github.io/tck
51 stars 36 forks source link

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

Closed StrayAlien closed 2 weeks ago

StrayAlien commented 5 months ago

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

baldimir commented 5 months ago

@dmn-tck/contributors please review this.

SimonRinguette commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 1 month ago

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

StrayAlien commented 1 month ago

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

opatrascoiu commented 2 weeks ago

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

baldimir commented 2 weeks ago

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

SimonRinguette commented 2 weeks ago

All good, merging