Fix variable.typeRef for BKMs #385
Conversation
| <variable typeRef="number" name="bkm_005"/> | ||
| <encapsulatedLogic> | ||
| <variable name="bkm_005"/> | ||
| <encapsulatedLogic typeRef="number"> |
There was a problem hiding this comment.
Hi @opatrascoiu, in this case, the actual purpose of the test is to assert what an impl does in this exact circumstance - it is not an oversight. The test description is "bkm type is non-list and bkm logic returns singleton list of conforming value - coercion list to val". In essence, the logic returns an untyped value and that is coerced to the type of the bkm variable. It is not testing whether an impl does type inference, that is tested elsewhere.
To maintain the intention of the test, I believe the type should remain where it is.
There was a problem hiding this comment.
I think the description of the test should be "bkm return type/ output type is non-list and bkm logic returns singleton list of conforming value - coercion list to val".
AFAIK variable.typeRef is the type of BKM (Any) -> number encapsulated.typeRef is the return type number.
I don't think moving the typeRef changes the intent of the test case.
There was a problem hiding this comment.
Hi @opatrascoiu. Merry Christmas to you. If a variable may have a type, and that type may cause a coercion, then it should be the subject of a test case. That is what this does. I think maybe the gap is that we may not have a test that does a similar thing to the typeRef on the encapsulated logic.
| <variable typeRef="tNumberList" name="bkm_004"/> | ||
| <encapsulatedLogic> | ||
| <variable name="bkm_004"/> | ||
| <encapsulatedLogic typeRef="tNumberList"> |
There was a problem hiding this comment.
Hi @opatrascoiu - as per the comments on bkm_005. The positioning of the type is part of the intention of the test. Though, I see the test for it is commented out referencing this: #238 (comment). I think that was a back-office change and not by me. It is maybe possible the intention of some tests that comment refers to may also have been misunderstood. Unsure.
| @@ -248,7 +248,7 @@ | |||
| </semantic:decision> | |||
| <semantic:businessKnowledgeModel id="_dc2ca64d-2044-4806-9d0e-e705b3b14447" name="Eligibility rules"> | |||
| <semantic:description><p><span style="font-family: arial, helvetica, sans-serif; font-size: 10pt;"><span lang="JA">The&nbsp;</span><strong><span lang="JA">Eligibility rules&nbsp;</span></strong><span lang="JA">decision logic&nbsp;defines a complete, priority-ordered single hit decision table&nbsp;</span>deriving Eligibility from Pre-Bureau Risk Category, Pre-Bureau Affordability and Age.</span></p></semantic:description> | |||
| <semantic:variable name="Eligibility rules" id="_fe2d2049-704b-4a88-88be-3d1a56c3d376" typeRef="tEligibility"/> | |||
There was a problem hiding this comment.
@opatrascoiu - this is the example from the spec. My feeling is removing these types from the variables throughout changes the nature of the bkms from one where a variable value may be type-coerced due to the typing on the variable, into one where the variable is type-inferred. So, whereas with the typings in place, there is a possibility that a null value will be returned because of null-coercion forced by the explicit type. With the types removed this is no longer the case.
There was a problem hiding this comment.
I know the example is from the spec, but in my opinion is incorrect. I raised an issue to be discussed in RTF.
Currently is in INBOX
https://issues.omg.org/browse/INBOX-1081
There was a problem hiding this comment.
Hi @opatrascoiu . That link requires a login I am afraid. Though, for the benefit of those without login creds there, I think this is it: https://issues.omg.org/issues/lists/unclassified#issue-47325. I'm not sure the position of the typeRef is incorrect according to that ticket, having a typeRef on the variable is permissible, but if the typedef of an invokable is supposed to be function type then yes, I get your point. Maybe leave the typedef where it is, but make them function types? (as per one of the options)
|
@opatrascoiu - regarding the three options, I don't think the first two are suitable as they change the meaning of tests (as noted above), but looking through the DMN spec, I can't quite see where it says that a BKM variable should be a function type. I'm not saying it should not, just that it is often hard to interpret the tea leaves. It does say it is a reference:
But does that mean it is a function type? |
|
Table 15: Invocable attributes and model associations page 62. @DanielThanner raised the initial issue (see #376), was discussed in TCK, was discussed in RTF, the agreement was that the type if present should be a function type. DMN 1.2 doesn't have function types, hence TFunctionItem was added in DMN 1.3. The purpose of this PR is to fix the debatable / incorrect ones and be consistent with the rests of the tests cases. All the other BKMs in the tests don't have a variable.typeRef |
|
Hi @opatrascoiu - that would be (yet another) breaking change for 1.3 then - not sure if the spec team take into account making breaking changes in a public spec - maybe breaking changes should be part of the release notes (if they are, apols, I have missed them). But okay. We need more tests there then. Though, I'm not sure the BKMs need to be 'consistent' in the TCK - having the typeRef on the variable is perfectly valid. IMO. |
|
@StrayAlien I agree we having typeRefs in BKM.variable is valid. I agree we need tests for TFunctionItem as well, its'a new DMN 1.3 feature that is not covered. I think we should create a new test suite dedicated to this aspect to keep them localized. I thought about adding them in this PR but in the end I decided to minimize the changes and removed typeRef to match existing test cases we agreed on (we agreed that BKM.variable.typeRef is optional). |
|
@opatrascoiu - maybe some of the tests could be be kept together, but I think they'll often belong with the test grouping - like equality, instance of, conformance - that sort of thing. Btw, writing the tests is not hard - getting a PR approved and merged is the hard bit ... I've got one there coming on 12 months old ... rather disheartening ... project feels dead. But there is a lot of 1.3 not yet covered. way more than just functions. EDIT: btw, it is a 'SHALL' that an invokable typeRef is a function type? Thats is, a model should be considered invalid it it is not a function type? EDIT 2: I'm getting off my lazy butt and just now implementing DMN 1.3, so may as well get some TCK tests in. Fingers crossed. I have updated our model verifier with the assertion that an invokable variable typeRef (if any) must refer to an itemDefinition with a functionItem - I get the same list you do. Sweet. Not sure of you noticed, but the 'parameters' child element of functionItem is the only element in the schema to be pluralised I think. Not a very consistent name at all. It should have been 'parameter'. So, now, every functionItem parameter element has to be called 'parameters' even though it represents just a single parameter .... nice. |
tarilabs
added
the
TCK TEST
|
So the proposal for identifying the return type, is to move it to the |
|
Hi @tarilabs - I forgot about this one. I think the PR solves the issue of the types not being function types in the current test suite, but that will still leave a gap in the TCK if indeed the typeRef of an invokable is to be a function type. If it so agreed I can get some tests in to assert it. |
|
@tarilabs Yes, that's correct. The type used in the bkm.variable.tyoeRef is the return / output type. @StrayAlien Would be great to have test cases for the function types. My preference is to have a new test suite (separate xml file) |
|
fyi, I have raised the plural nature of the FunctionItem |
This is to address #376
There are 3 ways to address above:
This request removes the typeRefs to be consistent with existing test cases.
We need to add extra test cases for remaning options (Any or proper TFunctionItem).