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

fix: Improve Control Renderer lint #392

Merged
merged 19 commits into from
Nov 13, 2024
Merged

fix: Improve Control Renderer lint #392

merged 19 commits into from
Nov 13, 2024

Conversation

d3xter666
Copy link
Contributor

@d3xter666 d3xter666 commented Oct 31, 2024

  • Detect non static renderer objects in ES6/TS code
  • Detect apiVersion property that is not part of the variable initialization
  • Properly analyze SAPUI5 raw renderers that might be ambient modules and present in @sapui5/types

@codeworrior codeworrior changed the title fix: Imporve Control Renderer lint fix: Improve Control Renderer lint Oct 31, 2024
src/linter/messages.ts Outdated Show resolved Hide resolved
src/linter/ui5Types/SourceFileLinter.ts Outdated Show resolved Hide resolved
@d3xter666 d3xter666 requested review from a team and codeworrior November 4, 2024 13:50
Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

Looks good, just some small remarks

src/linter/ui5Types/SourceFileLinter.ts Outdated Show resolved Hide resolved
src/linter/messages.ts Outdated Show resolved Hide resolved
@d3xter666 d3xter666 force-pushed the fix-renderer-findings branch from 89df70f to 3651db5 Compare November 12, 2024 12:35
@@ -333,6 +334,15 @@ export const MESSAGE_INFO = {
details: () => `"{@link sap.ui.core.RenderManager#methods/icon RenderManager}",`,
},

[MESSAGE.NOT_STATIC_CONTROL_RENDERER]: {
severity: LintMessageSeverity.Warning,
ruleId: RULES["no-deprecated-api"],
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect this an deprecated API, as it never was intended to be defined that way. Therefore I think we should use a different ruleId. Also, it is not related to UI5 2.x, but a general check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, thanks!
But, to me this one seems too specific. Would static-var-required be ok, then?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe ui5-class-declaration, as it is about how UI5 classes are declared/defined.
What do you think @codeworrior / @flovogt / @RandomByte ?

Copy link
Member

Choose a reason for hiding this comment

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

just another idea ui5-control-definition?

Copy link
Member

Choose a reason for hiding this comment

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

As the renderer is not a general concept for UI5 classes, I rather tend to ui5-control-declaration. But there might not be much in addition to this check that would justify a separate rule-ID for that. So I would be fine with ui5-class-declaration, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to ui5-class-declaration. Let me know if something else comes

src/linter/messages.ts Outdated Show resolved Hide resolved
src/linter/messages.ts Outdated Show resolved Hide resolved
src/linter/ui5Types/SourceFileLinter.ts Outdated Show resolved Hide resolved
@d3xter666 d3xter666 merged commit 8a3976f into main Nov 13, 2024
13 checks passed
@d3xter666 d3xter666 deleted the fix-renderer-findings branch November 13, 2024 08:16
@openui5bot openui5bot mentioned this pull request Nov 13, 2024
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