feat: type aggregation validation #164
Conversation
| xmlElement.parent, | ||
| model | ||
| ); | ||
| if (ui5class !== undefined && allowedTypeInAggregation !== undefined) { |
There was a problem hiding this comment.
flip condition and return empty array if we do not have enough data to compute
and at the last statement return the result of getInvalidAggregationTypeIssue
packages/xml-views-validation/src/validators/elements/type-of-aggregation.ts
Outdated
Show resolved
Hide resolved
| xmlns="sap.ui.commons"> | ||
| <m:Panel> | ||
| <m:headerToolbar> | ||
| <🢂Toolbar🢀></Toolbar> |
There was a problem hiding this comment.
ToolBar does not match the type of headerToolBa?
There was a problem hiding this comment.
maybe choose something else like a Button so its more obvious it should not match?
There was a problem hiding this comment.
This is not resolved.
The error message itself is misleading:
getMessage(INVALID_AGGREGATION_TYPE, "Toolbar", "Toolbar")
Can we please pick a more logical scenario to fail? e.g some Button inside headerToolbar
| ); | ||
| }); | ||
|
|
||
| it("will detect mismatch of class to explicit aggregation of 'UI5Interface' type", () => { |
There was a problem hiding this comment.
Should this description include the term "default aggregation"?
Also I do not think this is "explicit" case
There was a problem hiding this comment.
again this is not resolved, this is not an explicit aggregation case in this snippet
but instead an implicit (default) aggregation of Panel
| ); | ||
| }); | ||
|
|
||
| it("will detect mismatch of class to default aggregation type", () => { |
There was a problem hiding this comment.
the description seems inaccurate, this does not look like a default aggregation scenario
There was a problem hiding this comment.
The description seems inaccurate to me. I see no defualt aggregation in this snippet
| }); | ||
| }); | ||
|
|
||
| it("will not detect an issue when the class is not under aggregation", () => { |
There was a problem hiding this comment.
a class is always under an aggregation, in the case below it is under the Default aggregation of mvc:View.
There was a problem hiding this comment.
Again this is unresolved.
The description should be about "no issue when the types match"
packages/xml-views-validation/test/validators/element/type-of-aggregation-spec.ts
Outdated
Show resolved
Hide resolved
| const allowedTypeInAggregation = getValidAggregationTypeForSubNodes( | ||
| parentAggregation | ||
| ); | ||
| if (ui5class === undefined || allowedTypeInAggregation === undefined) { |
There was a problem hiding this comment.
The return [] for if (ui5class === undefined) should probably be done immediately after initializing the ui5Class variable
packages/xml-views-validation/src/validators/elements/type-of-aggregation.ts
Show resolved
Hide resolved
packages/xml-views-validation/src/validators/elements/type-of-aggregation.ts
Outdated
Show resolved
Hide resolved
| return aggregationType as UI5Class; | ||
| case "UI5Interface": | ||
| return aggregationType as UI5Interface; | ||
| /* istanbul ignore next - we can only recieve aggregation type of 'UI5Class' or 'UI5Interface' */ |
There was a problem hiding this comment.
Its probably true that the model does not contain any aggregation with an invalid type.
But that does not mean we could never receive one, just that we are unlikely to.
So perhaps we should create a test for this (with deepClone of the model).
But maybe the best solution would be to narrow down the type signature of UI5Aggregation?
@tal-sapan WDYT?
There was a problem hiding this comment.
If its not clear I meant that the UI5Aggregation.type would be narrowed down and be more specific UI5Class | UI5Interface and that the semanticModelBuilder would create a warn/error if that assumption is not met.
There was a problem hiding this comment.
There was a problem hiding this comment.
No. The type might be unresolved (e.g. if there is a typo) or there might be other errors in the api.json (and cases which I'm not even sure are errors, like primitive types, though I think they would be specified in altTypes instead). It should be handled in the code.
There was a problem hiding this comment.
- Representing unresolved in the model seems fine.
- But assuming an aggregation may not be of enum/primitive types, why should it even be represented in the model?
The model should not represent the API.json verbatim, rather it should represent the UI5 Semantics.
There was a problem hiding this comment.
I will investigate this outside the scope of this PR
packages/xml-views-validation/test/validators/element/type-of-aggregation-spec.ts
Outdated
Show resolved
Hide resolved
packages/xml-views-validation/test/validators/element/type-of-aggregation-spec.ts
Show resolved
Hide resolved
No description provided.