-
Notifications
You must be signed in to change notification settings - Fork 535
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
Improve error messages #3678
Improve error messages #3678
Conversation
This reverts commit b72e419
Done. 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. Fully reviewed this--just had 2 comments. I still can't approve until the wiki page is checked in, but the PR looks pretty close to done.
scripts/src/javatests/org/oppia/android/scripts/testfile/TestFileCheckTest.kt
Outdated
Show resolved
Hide resolved
scripts/src/javatests/org/oppia/android/scripts/xml/XmlSyntaxCheckTest.kt
Outdated
Show resolved
Hide resolved
Also, @Sparsh1212 please fix the CI checks & assign this back once the base PR is merged. |
* bazel run //scripts:accessibility_label_check -- $(pwd) app/src/main/AndroidManifest.xml | ||
* scripts/assets/accessibility_label_exemptions.pb | ||
* bazel run //scripts:accessibility_label_check -- $(pwd) | ||
* scripts/assets/accessibility_label_exemptions.pb app/src/main/AndroidManifest.xml |
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.
Missed Typo fixed now.
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! Just waiting for CI checks to pass before merging.
Unassigning @BenHenning since they have already approved the PR. |
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.
Looks like there's 1 CI failure. PTAL.
Hi @Sparsh1212, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks! |
@BenHenning I think all checks are passing here right? |
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. Nice find regarding the flake.
Explanation
Sample full error logs for all checks:
RegexPatternValidation Check:
File name/path violation: <failure_message>
filePath1:lineNumber1: errorToShow1
filePath2:lineNumber2: errorToShow1
filePath3:lineNumber3: errorToShow2
Refer to <wiki_url_1> for more details on how to fix this.
XmlSyntaxValidation Check:
errorFile1:lineNumber1:columnNumber1: error_message1
errorFile2:lineNumber2:columnNumber2: error_message2
errorFile3:lineNumber3:columnNumber3: error_message3
Refer to <wiki_url_2> for more details on how to fix this.
TestFilePresence Check:
File file_path_1 does not have a corresponding test file.
File file_path_2 does not have a corresponding test file.
File file_path_3 does not have a corresponding test file.
Refer to <wiki_url_3> for more details on how to fix this.
AccessibilityLabel Check:
Accessibility label missing for Activities:
Refer to <wiki_url_4> for more details on how to fix this.
KDocValidity Check:
KDoc missing for files:
Refer to <wiki_url_5> for more details on how to fix this.
TodoOpen Check:
TODOs not in proper format:
TODOs not corresponding to open issues on GitHub:
Refer to <wiki_url_6> for more details on how to fix this.
TodoIssueResolved Check:
The following TODOs are unresolved for the closed issue:
Refer to <wiki_url_7> for more details on how to fix this.
Checklist