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] JSDoc: add project name to JSDoc configuration and api.json #609

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

codeworrior
Copy link
Member

@codeworrior codeworrior commented Jun 3, 2021

The JSDoc processor did not propagate the library name (project name) to the JSDoc configuration. As a consequence, the UI5 template (publish.js) could not properly validate the metadata property 'library' for control and element classes. It reported many false positives ("ERROR: specified library 'foo.bar' for class 'foo.bar.Control' doesn't match containing library 'undefined'").

The JSDoc processor did not propagate the library name (project name) to
the JSDoc configuration. As a consequence, the UI5 template (publish.js)
could not properly validate the metadata property 'library' for control
and elements classes. It reported many false positives ("ERROR:
specified library 'foo.bar' for class 'foo.bar.Control' doesn't match
containing library 'undefined'").
@codeworrior codeworrior requested a review from matz3 June 3, 2021 05:14
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.72% when pulling 65c053d on fix-library-name-in-api.json into 97ac08b on master.

@codeworrior codeworrior merged commit 419ce38 into master Jun 3, 2021
@codeworrior codeworrior deleted the fix-library-name-in-api.json branch June 3, 2021 21:24
@codeworrior
Copy link
Member Author

@RandomByte thank you for the quick review!

@@ -106,6 +106,7 @@ async function generateJsdocConfig({targetPath, tmpPath, namespace, projectName,
"ui5": {
"variants": ${JSON.stringify(variants)},
"version": "${version}",
"uilib": "${projectName}",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not use the projectName (= ui5.yaml metadata name) but the namespace.
Usually they are equal but the namespace is guaranteed to be correct as it is also used for the folder structure. The project name is mainly used for logging purposes.

See my abandoned change here: https://github.com/SAP/ui5-builder/pull/598/files#diff-03d08225d447cd18da349dd9928b9093a83e66d9a6aa66cee4e0a0038779a9c8

Copy link
Member

Choose a reason for hiding this comment

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

Hm, what does "correct" mean here anyways? If the "uilib" property shall contain the libraries name, the projectName is always correct imho.

Copy link
Member

Choose a reason for hiding this comment

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

Especially for that "library" check IMO it's important that it fits to the namespace of the library. If it would be something else (e.g. mylib instead of com.example.mylib), the check would fail and report false findings.

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case we should change it to the namespace

@matz3
Copy link
Member

matz3 commented Jun 7, 2021

Thanks! I only cherry-picked the openui5 change and missed to add my changes from #598.

matz3 added a commit that referenced this pull request Jun 7, 2021
Tests have been adopted to check whether the right information is used.

Follow-up of #609
matz3 added a commit that referenced this pull request Jun 8, 2021
Tests have been adopted to check whether the right information is used.

Follow-up of #609
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.

4 participants