-
Notifications
You must be signed in to change notification settings - Fork 528
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 #3304: Add check to ensure KDoc presence for non-private members #3499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212! I think this looks really great--the test name changes make them much clearer, I think.
I just noticed one comment didn't seem resolved, but I otherwise have nothing new to add.
Could you please fix the deltas on this PR & its title since #3352 was merged?
scripts/src/javatests/org/oppia/android/scripts/docs/KDocValidityCheckTest.kt
Outdated
Show resolved
Hide resolved
Thanks @BenHenning the PR is now updated and the one left a comment is also addressed. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212
Took a quick pass on this. Left few suggestions. Also, please check on failing actions.
scripts/src/java/org/oppia/android/scripts/docs/KdocValidityCheck.kt
Outdated
Show resolved
Hide resolved
scripts/src/javatests/org/oppia/android/scripts/docs/KdocValidityCheckTest.kt
Outdated
Show resolved
Hide resolved
scripts/src/javatests/org/oppia/android/scripts/docs/KdocValidityCheckTest.kt
Show resolved
Hide resolved
scripts/src/javatests/org/oppia/android/scripts/docs/KdocValidityCheckTest.kt
Show resolved
Hide resolved
Thanks, @anandwana001 followed up on all the suggestions. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Sparsh1212! LGTM.
scripts/src/javatests/org/oppia/android/scripts/docs/KdocValidityCheckTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Explanation
Fixes #3304: Add check to ensure KDoc presence for non-private members
Script for ensuring that KDocs are present on all non-private:
Note: If any of the above member has an override modifier present, then it is automatically
exempted for the KDoc check.
Usage:
bazel run //scripts:kdoc_check -- <path_to_directory_root>
Arguments:
Example:
bazel run //scripts:kdoc_check -- $(pwd)
For testing the script, automated tests have been added.
To execute the tests, use:
bazel test //scripts/src/javatests/org/oppia/android/scripts/kdoc:KDocCheckTest
Note: We are generating the test assets dynamically at the time of executing them. The test assets are automatically deleted when the test finishes.
SS when the CI check fails:
Checklist