feat: Add detection for deprecated dependencies in .library #104

maxreichmann · 2024-05-07T15:59:53Z

JIRA: CPOUI5FOUNDATION-825

+refactor: Add common xmlParser
+refactor: Remove htmlParser

The Lib.init() call detection is addressed in the following PR: #197

JIRA: CPOUI5FOUNDATION-825 The `.library` detection is addressed in the following PR: #104

src/linter/dotLibrary/DotLibraryLinter.ts

d3xter666

LGTM, but we worked in pair on that, so someone else needs to take a look, too

src/linter/dotLibrary/linter.ts

matz3 · 2024-07-17T15:26:49Z

When limiting the linting to just the .library, the linter fails, which might be related to @flovogt's comment about the root reader:

ui5lint --file-paths src/sap/f/.library
⚠️  Process Failed With Error

Error Message:
Resource not found: /resources/sap/f/.library

Stack Trace:
Error: Resource not found: /resources/sap/f/.library
    at lib/linter/dotLibrary/linter.js:14:23

matz3 · 2024-07-17T15:27:58Z

When there is an empty node <libraryName></libraryName>, there is an exception reported:

openui5/src/sap.f/src/sap/f/.library
0:0 error Fatal error: Cannot read properties of undefined (reading 'value')

matz3 · 2024-07-17T15:32:24Z

The library name node is looked up everywhere in the XML document, not just within the expected structure.
So putting a <libraryName>sap.ui.commons</libraryName> anywhere also leads to an error, which isn't really expected. In this case there is no conflict, as the name is not used anywhere else in the document.

Could you check how much effort it would be to really check for the right place of the dependency name in the document?
Only such a definition should be checked:

<?xml version="1.0" encoding="UTF-8" ?>
<library xmlns="http://www.sap.com/sap.ui.library.xsd" >
  <dependencies>
    <dependency>
      <libraryName>sap.ui.commons</libraryName>
    </dependency>
  </dependencies>
</library>

As this is not expected to cause any issues, I'm fine with the current state in case this is a high effort to implement.

d3xter666 · 2024-07-29T08:55:32Z

When limiting the linting to just the .library, the linter fails, which might be related to @flovogt's comment about the root reader:
ui5lint --file-paths src/sap/f/.library
⚠️  Process Failed With Error

Error Message:
Resource not found: /resources/sap/f/.library

Stack Trace:
Error: Resource not found: /resources/sap/f/.library
    at lib/linter/dotLibrary/linter.js:14:23

Fixed with a876b14

…file

d3xter666 · 2024-07-29T10:16:01Z

not read properties of undefined (reading 'va

Fixed

d3xter666 · 2024-07-29T10:16:37Z

The library name node is looked up everywhere in the XML document, not just within the expected structure. So putting a <libraryName>sap.ui.commons</libraryName> anywhere also leads to an error, which isn't really expected. In this case there is no conflict, as the name is not used anywhere else in the document.

Could you check how much effort it would be to really check for the right place of the dependency name in the document? Only such a definition should be checked:
<?xml version="1.0" encoding="UTF-8" ?>
<library xmlns="http://www.sap.com/sap.ui.library.xsd" >
  <dependencies>
    <dependency>
      <libraryName>sap.ui.commons</libraryName>
    </dependency>
  </dependencies>
</library>
As this is not expected to cause any issues, I'm fine with the current state in case this is a high effort to implement.

We've managed to handle this one with 12bfaec

matz3

LGTM. The mentioned issue have been addressed. Just some minor remarks.

matz3 · 2024-07-29T13:24:45Z

src/linter/dotLibrary/linter.ts

+			const resource = await workspace.byPath(resourcePath);
+			if (!resource) {
+				throw new Error(`Resource not found: ${resourcePath}`);
+			}
+			dotLibraryResources.push(resource);


This is not covered by the tests

matz3 · 2024-07-29T13:26:01Z

src/linter/dotLibrary/DotLibraryLinter.ts

+			this.#context.addLintingMessage(this.#resourcePath, {
+				severity: LintMessageSeverity.Error,
+				message,
+				ruleId: RULES["ui5-linter-parsing-error"],
+				fatal: true,
+			});


Not covered by unit tests

src/linter/dotLibrary/DotLibraryLinter.ts

🚜 New release prepared --- ## [0.3.1](v0.3.0...v0.3.1) (2024-07-30) ### Features * Add detection for deprecated dependencies in .library ([#104](#104)) ([161f157](161f157)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

maxreichmann changed the title ~~feat: Add Linter for .library files~~ feat: Add detection for deprecated dependencies in .library + library.js files May 7, 2024

matz3 mentioned this pull request May 27, 2024

Roadmap: v1.0.0 #21

Closed

27 tasks

maxreichmann changed the title ~~feat: Add detection for deprecated dependencies in .library + library.js files~~ feat: Add detection for deprecated dependencies in .library Jul 12, 2024

maxreichmann force-pushed the feature-825-analyze-library.js-&-.library branch from 759be35 to 0cd78fa Compare July 12, 2024 12:02

d3xter666 mentioned this pull request Jul 12, 2024

feat: Detect deprecated deps in Lib.init call #197

Merged

d3xter666 added a commit that referenced this pull request Jul 17, 2024

feat: Detect deprecated deps in Lib.init call (#197)

9c9c406

JIRA: CPOUI5FOUNDATION-825 The `.library` detection is addressed in the following PR: #104

maxreichmann marked this pull request as ready for review July 17, 2024 08:26

maxreichmann marked this pull request as draft July 17, 2024 08:26

maxreichmann and others added 14 commits July 17, 2024 10:27

feat: Add Linter for .library files

9c019b0

feat: Add xml parser

a05bdfe

fix: Eslint issues

430ac47

fix: Remove eslint annotations

e7a0bba

feat: Detect depr. libs within .library files

d78f1a7

refactor: Rename variables

8b262ac

fix: Eslint issues

f2e1065

test: Update snapshots

da44641

fix: Eslint issues

610302f

fix: Remove debug leftover

ca7a962

test: Add test for xmlParser

e9bd8af

fix: Adjust test for xmlParser

2a6533d

refactor: Clean up

442ae21

test: Update snapshots

ce5dd32

maxreichmann force-pushed the feature-825-analyze-library.js-&-.library branch from a7b646e to ce5dd32 Compare July 17, 2024 08:36

maxreichmann marked this pull request as ready for review July 17, 2024 08:40

maxreichmann requested a review from a team July 17, 2024 08:41

d3xter666 reviewed Jul 17, 2024

View reviewed changes

src/linter/dotLibrary/DotLibraryLinter.ts Outdated Show resolved Hide resolved

d3xter666 reviewed Jul 17, 2024

View reviewed changes

src/linter/dotLibrary/DotLibraryLinter.ts Outdated Show resolved Hide resolved

maxreichmann added 2 commits July 17, 2024 12:32

refactor: Use util consts in linter messages

8f7284a

refactor: Use util const

a6c2dac

d3xter666 reviewed Jul 17, 2024

View reviewed changes

maxreichmann requested a review from a team July 17, 2024 11:01

flovogt reviewed Jul 17, 2024

View reviewed changes

src/linter/dotLibrary/linter.ts Outdated Show resolved Hide resolved

fix: Use workspace reader instead of the rootReader

a876b14

d3xter666 added 6 commits July 29, 2024 12:16

fix: Check for empty textNodes within libraryName tag

0f86b32

refactor: Extend xmlParser to support also openTags

40af990

refactor: Detect libs only when positioned correctly within .library …

12bfaec

…file

fix: Correct Glob pattern for .library files

d601d26

test: Add test for the additional cases

6d9f896

test: Update snapshots

e64b03a

matz3 reviewed Jul 29, 2024

View reviewed changes

d3xter666 added 4 commits July 29, 2024 16:49

refactor: Provide correct ruleId

9dc5ccc

refactor: Remove trim from the check

e78d5d4

refactor: Remove empty lib check

c76dce5

refactor: Remove slicing from stack check

3971889

matz3 approved these changes Jul 29, 2024

View reviewed changes

maxreichmann merged commit 161f157 into main Jul 30, 2024
13 checks passed

maxreichmann deleted the feature-825-analyze-library.js-&-.library branch July 30, 2024 07:49

openui5bot mentioned this pull request Jul 30, 2024

release: v0.3.1 #228

Merged

matz3 mentioned this pull request Aug 7, 2024

refactor(SourceFileReporter): Improve addMessage signature #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add detection for deprecated dependencies in .library #104

feat: Add detection for deprecated dependencies in .library #104

maxreichmann commented May 7, 2024 •

edited

Loading

d3xter666 left a comment

matz3 commented Jul 17, 2024

matz3 commented Jul 17, 2024

matz3 commented Jul 17, 2024

d3xter666 commented Jul 29, 2024

d3xter666 commented Jul 29, 2024

d3xter666 commented Jul 29, 2024

matz3 left a comment

matz3 Jul 29, 2024

matz3 Jul 29, 2024

feat: Add detection for deprecated dependencies in .library #104

feat: Add detection for deprecated dependencies in .library #104

Conversation

maxreichmann commented May 7, 2024 • edited Loading

d3xter666 left a comment

Choose a reason for hiding this comment

matz3 commented Jul 17, 2024

matz3 commented Jul 17, 2024

matz3 commented Jul 17, 2024

d3xter666 commented Jul 29, 2024

d3xter666 commented Jul 29, 2024

d3xter666 commented Jul 29, 2024

matz3 left a comment

Choose a reason for hiding this comment

matz3 Jul 29, 2024

Choose a reason for hiding this comment

matz3 Jul 29, 2024

Choose a reason for hiding this comment

maxreichmann commented May 7, 2024 •

edited

Loading