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

[INTERNAL] JSDoc: Add check for copyright notice #617

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Jun 15, 2021

Checks for a copyright notice at the beginning of each file.
The comment doesn't need to be the first comment but it must be placed
before any code starts.

This implements the same check as the internal validation rule
DOCUMENTATION__COPYRIGHT_NOTICE_MISSING.

Adds a jsdoc exclude configuration for sap.ui.model.odata.datajs which
is a legacy redirect file that only exists in the internal core overlay.
It is configured to be excluded from the internal validation, so it also
needs to be excluded from the jsdoc build so that no error is caused.

JIRA: CPOUI5FOUNDATION-329

Cherry picked from SAP/openui5@bcb5110

@coveralls
Copy link

coveralls commented Jun 15, 2021

Coverage Status

Coverage remained the same at 94.721% when pulling 0580905 on jsdoc-plugin-check-copyright-notice into b86f0fa on master.

Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

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

The change itself looks good to me. But please rewrite the commit message before submit as it mentions a change in the openui5 code (.library) which is not present in this commit here.

codeworrior
codeworrior previously approved these changes Jun 15, 2021
Copy link
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

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

U-Turn 1:
Mhmm, after thinking again about it, I withdraw my approval.

Isn't this a BREAKING change? Users of the ui5 tooling so far did not get an error when their files did not contain a copyright. The SAP internal UI5 validation did not run for their projects...

Internally, we have more control about whether a JSDoc error breaks the build or not.

U-Turn 2:
I tried it out now locally and to my surprise, the build did not break on errors. neither in the buld-sdk variant nor when using the @ui5/cli directly.
So we can continue with this change, but should have another look at the exit code of our JSDoc build.

Checks for a copyright notice at the beginning of each file.
The comment doesn't need to be the first comment but it must be placed
before any code starts.

This implements the same check as the internal validation rule
DOCUMENTATION__COPYRIGHT_NOTICE_MISSING.

JIRA: CPOUI5FOUNDATION-329

Cherry picked from SAP/openui5@bcb5110
@matz3
Copy link
Member Author

matz3 commented Jun 16, 2021

Thanks. I've updated the commit message.

Yeah it seems that exit codes 0 and 1 are both resolving the promise. I couldn't find a hint why this was done in #42.

@matz3
Copy link
Member Author

matz3 commented Jun 16, 2021

I've added a bullet-point to the list of breaking changes for 3.0

@codeworrior
Copy link
Member

codeworrior commented Jun 16, 2021

... I couldn't find a hint why this was done in #42.

I think with older versions of JSDoc, we observed the same exit code for success and error. I can't say whether this was still the case for JSDoc 3.5.5 (with which we started in #42), but I guess that was the reason why we ignored the exit code.

[Update]
Thinking about it, it might even be that we misunderstood the logging behavior of JSDoc at that time (errors result in an exit code of 1) and our JSDoc wasn't cleaned up enough.

Anyhow, we now rely on the exit code in the SAP internal build and should rely on it in the tooling as well (breaking -> 3.0)

@matz3 matz3 requested a review from codeworrior June 16, 2021 07:38
@matz3 matz3 merged commit 2a1ad72 into master Jun 16, 2021
@matz3 matz3 deleted the jsdoc-plugin-check-copyright-notice branch June 16, 2021 07:50
matz3 added a commit that referenced this pull request Jun 16, 2021
matz3 added a commit that referenced this pull request Jun 16, 2021
@matz3
Copy link
Member Author

matz3 commented Nov 18, 2022

Anyhow, we now rely on the exit code in the SAP internal build and should rely on it in the tooling as well (breaking -> 3.0)

Addressed via #845

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.

3 participants