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.

Sign up for GitHub

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

Jump to bottom

refactor: Refactoring of detect Component best practices PR #127

Merged
merged 7 commits into from
May 28, 2024
Merged

refactor: Refactoring of detect Component best practices PR #127

merged 7 commits into from
May 28, 2024

Conversation

RandomByte
Copy link
Member

No description provided.

@RandomByte
refactor(tests): Rename BestPractices to AsyncComponentFlags, use dir…
a193ba9 
… name as test name

Using the dir name as test name allows for use of 'only_' modifiers to
control which tests are executed.
@RandomByte
refactor: Rename bestPractices module to asyncComponentFlags, also ch…
0a5ce43 
…eck TS-Components
@RandomByte
test(AsyncComponentFlags): Add more test cases
6a86827 
Snapshots reflect unexpected behavior. Negative tests should have no
findings while positive tests should show findings.
@RandomByte
refactor(asyncComponentFlags): Remove usage of internal API, improve …
d9754c5 
…string extraction
@RandomByte
refactor(asyncComponentFlags): Rename types, use boolean for async in…
624c15d 
…terface flag

Since the async interface is either used or not, the information whether
it is "not set" is redundant and the "parent property not set"
information is not applicable.
@RandomByte
refactor(asyncComponentFlags): Simplify async flag merge logic
08288f7 
Use enum value for implicit searching
@RandomByte RandomByte requested a review from d3xter666 May 27, 2024 16:14
@RandomByte
Copy link
Member Author

Test case Positive_11 seems to not detect the missing async flags/interface. Need to look into this tomorrow

d3xter666
d3xter666 reviewed May 27, 2024
View reviewed changes
src/linter/ui5Types/asyncComponentFlags.ts
if (!importClause.namedBindings) {
// Example: import "sap/ui/core/library"; or import library from "sap/ui/core/library";
} else if (ts.isNamedImports(importClause.namedBindings)) {
// Example: import { IAsyncContentCreation } from "sap/ui/core/library";
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also an edge case here- to rename the import 😅
import { IAsyncContentCreation: myRenamedAsyncFlag } from ...

@d3xter666
Copy link
Contributor

Test case Positive_11 seems to not detect the missing async flags/interface. Need to look into this tomorrow

The issue is that the Component does not have members (https://github.com/SAP/ui5-linter/pull/127/files#diff-b33fcef6fb0b205372cd4799b7486a23abf3f17b4461bc35e21a0466ad1e9179R89), therefore the check for the manifest.json gets skipped.
The manifest is not propagetd to the parent component analysis and it never got checked: https://github.com/SAP/ui5-linter/pull/127/files#diff-b33fcef6fb0b205372cd4799b7486a23abf3f17b4461bc35e21a0466ad1e9179R106
It should have been working before as a side effect of the propagation to the parent component.

@RandomByte RandomByte force-pushed the component-best-practices-tests branch from d4261e8 to a1c7b67 Compare May 28, 2024 07:53
@RandomByte
Copy link
Member Author

Test case Positive_11 seems to not detect the missing async flags/interface. Need to look into this tomorrow

The issue is that the Component does not have members (https://github.com/SAP/ui5-linter/pull/127/files#diff-b33fcef6fb0b205372cd4799b7486a23abf3f17b4461bc35e21a0466ad1e9179R89), therefore the check for the manifest.json gets skipped. The manifest is not propagetd to the parent component analysis and it never got checked: https://github.com/SAP/ui5-linter/pull/127/files#diff-b33fcef6fb0b205372cd4799b7486a23abf3f17b4461bc35e21a0466ad1e9179R106 It should have been working before as a side effect of the propagation to the parent component.

Great, thanks! Changed the fixture, and even fixed a bug Unhandled CallExpression expression syntax: SuperKeyword

@RandomByte RandomByte merged commit 992dee4 into component-best-practices May 28, 2024
2 checks passed
@RandomByte RandomByte deleted the component-best-practices-tests branch May 28, 2024 09:34
@RandomByte RandomByte mentioned this pull request May 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d3xter666 d3xter666 d3xter666 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RandomByte @d3xter666