-
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
Fixed #4396, #4883 : Add CI checks to ensure Kotlin files , Layout xml files are only referencing colors from component_colors.xml #4882
Fixed #4396, #4883 : Add CI checks to ensure Kotlin files , Layout xml files are only referencing colors from component_colors.xml #4882
Conversation
…from component_colors.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.
@BenHenning PTAL, the check is working fine for all xml files as you see in the CI check failure.
And also Kotlin files checks are failing.
… component_colors.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.
This looks reasonable @MohitGupta121. Is there anything you want me to look at besides the patterns?
I don't think anything else expect the Path_Pattern. And nothing from the issue side also as I see. @BenHenning What do you think ? |
@BenHenning PTAL, Combine kotlin files checks into 1 check |
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 @MohitGupta121. Do you have examples for views that use 'View' as a prefix that would need updating?
Also, please update the PR description to have the fixed issues as separate lines (otherwise GitHub doesn't hook them up to automation correctly).
@BenHenning Yes got the views whose prefix is "View" https://github.com/oppia/oppia-android/tree/develop/app/src/main/java/org/oppia/android/app/customview |
@BenHenning PTAL,
At present view not have ViewPresenter so not have any example as such but added Test in regex. |
Hi @MohitGupta121, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
…nto color_ci_checks_for_xml_files
scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.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.
@BenHenning PTAL, Thanks.
I updated all the Regex Tests and now working for all kotlin as well as xml.
Please tell me if we need to change or update anything else.
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 @MohitGupta121! This LGTM!
scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt
Show resolved
Hide resolved
scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt
Outdated
Show resolved
Hide resolved
Hi @MohitGupta121, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Explanation
Fixed #4396 : Add CI checks to ensure Layout xml files are only referencing colors from component_colors.xml
Fixed #4883 : Add CI checks to ensure Kotlin Files (Activities, Activity Presenters, Fragments, Fragment Presenters, Views, and View Presenters) are only referencing colors from component_colors.xml
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: