-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Detect inline XML fragments & views (reuse TS program) #519
base: main
Are you sure you want to change the base?
Conversation
5205e53
to
ae136f2
Compare
a196c8b
to
480249c
Compare
Note: This change sorts messages in the tests, so that many unrelated test snapshots are also included in here. |
Currently, the linting results are deterministic and we should stick with that. Only sorting the results when comparing within unit tests will not lead to deterministic results for public consumers. I suspect that this is caused by a missing sorting of the newly created files. For the initial files, this is done via: ui5-linter/src/linter/ui5Types/TypeLinter.ts Lines 87 to 88 in 70ad529
|
@@ -204,7 +204,8 @@ export default class SourceFileLinter { | |||
if (ts.isPropertyAssignment(prop) && | |||
(ts.isIdentifier(prop.name) || ts.isStringLiteralLike(prop.name)) && | |||
ts.isStringLiteralLike(prop.initializer) && | |||
["definition", "fragmentContent", "viewContent"].includes(prop.name.text)) { | |||
["definition", "fragmentContent", "viewContent"].includes(prop.name.text) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same check is done again below in isXmlInJsCreationCall
isn't it?
I wonder whether it would be easier to combine isXmlInJsCreationCall
and getViewTypeFromXMLInJsCreationCall
as they are very similar. Also, as a CallExpression is required, wouldn't it be better to use the CallExpression as indication node to start the checking instead of the isObjectLiteralExpression inside?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that this fits into the same category as the partial deprecations, where we perform checks for a specific function call of a module / class.
// This resource has already been transpiled & linted, so no need for duplicate linting | ||
if (contextMeta.xmlCompiledResource) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether this is the best way to only lint the new files. It currently causes files that have parsing errors like "Unexpected aggregation within an aggregation" to be linted twice. Ideally, we only pass the list of resources that should be linted from outside.
@@ -204,7 +204,8 @@ export default class SourceFileLinter { | |||
if (ts.isPropertyAssignment(prop) && | |||
(ts.isIdentifier(prop.name) || ts.isStringLiteralLike(prop.name)) && | |||
ts.isStringLiteralLike(prop.initializer) && | |||
["definition", "fragmentContent", "viewContent"].includes(prop.name.text)) { | |||
["definition", "fragmentContent", "viewContent"].includes(prop.name.text) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that this fits into the same category as the partial deprecations, where we perform checks for a specific function call of a module / class.
JIRA: CPOUI5FOUNDATION-986
This is alternative implementation of #517 by reusing
program
instead of TS transpiling file by file.Note: This change's base is currently #518 as it paves the ground for dynamic addition of resources to the TS program