Fix #3786: Fix regex checks for translated strings #3787
Conversation
Also, performance improvements for the regex check.
| @Test | ||
| fun testFileContent_translatableString_inTranslatedPrimaryStringsFile_fileContentIsCorrect() { | ||
| val prohibitedContent = "<string name=\"test\">Translatable</string>" | ||
| tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values-ar") |
There was a problem hiding this comment.
can we have a test case for
app/src/main/res/values-ar/<str>/strings.xml too
There was a problem hiding this comment.
Is this actually allowed by Android resources? I was under the impression that value files should generally live directly under their respective values folders?
There was a problem hiding this comment.
Hmm https://stackoverflow.com/questions/4930398/can-the-android-layout-folder-contain-subfolders lays out contradictory information. I'm actually thinking that maybe we need a check to prohibit subdirectories instead of supporting subdirectories in these types of patterns.
There was a problem hiding this comment.
This is the new android studio template for new projects
I guess it is allowed
There was a problem hiding this comment.
@FareesHussain is that representing directory structure? That doesn't seem right at face value since themes.xml (night) means values-night/themes.xml. What's the directory structure for that folder ('project view' not 'Android view')?
There was a problem hiding this comment.
Ah my bad, it is values-night/themes.xml
There was a problem hiding this comment.
New check added.
|
Unassigning @FareesHussain since the review is done. |
|
New check has been added. Thanks @FareesHussain! PTAL. |
|
Hi @BenHenning, 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! |
|
Thanks again @FareesHussain! |
Explanation
Fixes #3786
#3754 introduced new regex checks to make sure translated strings go into the correct XML files, but the check didn't quite work for translated versions of strings.xml. However, the existing regex check didn't have support for matching against file patterns when exempting files (which is needed in this case since the check should exclude translated values directories).
The changes to the check include:
Note that one behavior change with the changes above is that failures will now be organized by file rather than by check (i.e. all failures for a particular file will be printed in order of that file's lines rather than in order of the checks).
See #3764 for a passing demonstration of the check (since the check is currently passing on develop; this is only an issue when trying to merge #3687--the test branch combines this & that branch). Also, new tests were added (one specifically for validating this case). See this test fail/pass without/with the regex change:
Test failing with the old pattern
Test passing with the new pattern
Finally, per the review an additional check was added to prohibited nested resource directories to simplify the set of potential matches for strings.xml (i.e. to ensure strings.xml must live immediately under a 'values--r/' directory structure).
Essential Checklist
For UI-specific PRs only
N/A