diff --git a/scripts/assets/file_content_validation_checks.textproto b/scripts/assets/file_content_validation_checks.textproto index 27552f874d7..6399ace7b2a 100644 --- a/scripts/assets/file_content_validation_checks.textproto +++ b/scripts/assets/file_content_validation_checks.textproto @@ -100,5 +100,5 @@ file_content_checks { file_path_regex: ".+?.xml" prohibited_content_regex: "" failure_message: "All strings outside strings.xml must be marked as not translatable, or moved to strings.xml." - exempted_file_name: "app/src/main/res/values/strings.xml" + exempted_file_patterns: "app/src/main/res/values.*?/strings\\.xml" } diff --git a/scripts/assets/filename_pattern_validation_checks.textproto b/scripts/assets/filename_pattern_validation_checks.textproto index fc6cad22b44..9307ed6f21d 100644 --- a/scripts/assets/filename_pattern_validation_checks.textproto +++ b/scripts/assets/filename_pattern_validation_checks.textproto @@ -1,4 +1,8 @@ filename_checks { prohibited_filename_regex: "^((?!(app|testing)).)+/src/main/.+?Activity.kt" - failure_message: "Activities cannot be placed outside the app or testing module" + failure_message: "Activities cannot be placed outside the app or testing module." +} +filename_checks { + prohibited_filename_regex: "^.+?/res/([^/]+/){2,}[^/]+$" + failure_message: "Only one level of subdirectories under res/ should be maintained (further subdirectories aren't supported by the project configuration)." } diff --git a/scripts/src/java/org/oppia/android/scripts/proto/file_content_validation_checks.proto b/scripts/src/java/org/oppia/android/scripts/proto/file_content_validation_checks.proto index 8e439361a63..369918fe75b 100644 --- a/scripts/src/java/org/oppia/android/scripts/proto/file_content_validation_checks.proto +++ b/scripts/src/java/org/oppia/android/scripts/proto/file_content_validation_checks.proto @@ -24,4 +24,7 @@ message FileContentCheck { // Names of all the files which should be exempted for this check. repeated string exempted_file_name = 4; + + // Regex patterns for all file/file paths that should be exempted for this check. + repeated string exempted_file_patterns = 5; } diff --git a/scripts/src/java/org/oppia/android/scripts/regex/RegexPatternValidationCheck.kt b/scripts/src/java/org/oppia/android/scripts/regex/RegexPatternValidationCheck.kt index e3826a45d1f..464251b2689 100644 --- a/scripts/src/java/org/oppia/android/scripts/regex/RegexPatternValidationCheck.kt +++ b/scripts/src/java/org/oppia/android/scripts/regex/RegexPatternValidationCheck.kt @@ -32,24 +32,25 @@ fun main(vararg args: String) { // Check if the repo has any filename failure. val hasFilenameCheckFailure = retrieveFilenameChecks() - .fold(initial = false) { isFailing, filenameCheck -> - val checkFailed = checkProhibitedFileNamePattern( + .fold(initial = false) { hasFailingFile, filenameCheck -> + val fileFails = checkProhibitedFileNamePattern( repoRoot, searchFiles, filenameCheck, ) - isFailing || checkFailed + return@fold hasFailingFile || fileFails } // Check if the repo has any file content failure. - val hasFileContentCheckFailure = retrieveFileContentChecks() - .fold(initial = false) { isFailing, fileContentCheck -> - val checkFailed = checkProhibitedContent( + val contentChecks = retrieveFileContentChecks().map { MatchableFileContentCheck.createFrom(it) } + val hasFileContentCheckFailure = + searchFiles.fold(initial = false) { hasFailingFile, searchFile -> + val fileFails = checkProhibitedContent( repoRoot, - searchFiles, - fileContentCheck + searchFile, + contentChecks ) - isFailing || checkFailed + return@fold hasFailingFile || fileFails } if (hasFilenameCheckFailure || hasFileContentCheckFailure) { @@ -138,38 +139,33 @@ private fun checkProhibitedFileNamePattern( * Checks for a prohibited file content. * * @param repoRoot the root directory of the repo - * @param searchFiles a list of all the files which needs to be checked - * @param fileContentCheck proto object of FileContentCheck + * @param searchFile the file to check for prohibited content + * @param fileContentChecks contents to check for validity * @return whether the file content pattern is correct or not */ private fun checkProhibitedContent( repoRoot: File, - searchFiles: List, - fileContentCheck: FileContentCheck + searchFile: File, + fileContentChecks: Iterable ): Boolean { - val filePathRegex = fileContentCheck.filePathRegex.toRegex() - val prohibitedContentRegex = fileContentCheck.prohibitedContentRegex.toRegex() - - val matchedFiles = searchFiles.filter { file -> - val fileRelativePath = file.toRelativeString(repoRoot) - val isExempted = fileRelativePath in fileContentCheck.exemptedFileNameList - return@filter if (!isExempted && filePathRegex.matches(fileRelativePath)) { - file.useLines { lines -> - lines.foldIndexed(initial = false) { lineIndex, isFailing, lineContent -> - val matches = prohibitedContentRegex.containsMatchIn(lineContent) - if (matches) { - logProhibitedContentFailure( - lineIndex + 1, // Increment by 1 since line numbers begin at 1 rather than 0. - fileContentCheck.failureMessage, - fileRelativePath - ) - } - isFailing || matches + val lines = searchFile.readLines() + return fileContentChecks.fold(initial = false) { hasFailingFile, fileContentCheck -> + val fileRelativePath = searchFile.toRelativeString(repoRoot) + val fileFails = if (fileContentCheck.isFileAffectedByCheck(fileRelativePath)) { + val affectedLines = fileContentCheck.computeAffectedLines(lines) + if (affectedLines.isNotEmpty()) { + affectedLines.forEach { lineIndex -> + logProhibitedContentFailure( + lineIndex + 1, // Increment by 1 since line numbers begin at 1 rather than 0. + fileContentCheck.failureMessage, + fileRelativePath + ) } } + affectedLines.isNotEmpty() } else false + return@fold hasFailingFile || fileFails } - return matchedFiles.isNotEmpty() } /** @@ -208,3 +204,46 @@ private fun logProhibitedContentFailure( val failureMessage = "$filePath:$lineNumber: $errorToShow" println(failureMessage) } + +/** A matchable version of [FileContentCheck]. */ +private data class MatchableFileContentCheck( + val filePathRegex: Regex, + val prohibitedContentRegex: Regex, + val failureMessage: String, + val exemptedFileNames: List, + val exemptedFilePatterns: List +) { + /** + * Returns whether the relative file given by the specified path should be affected by this check + * (i.e. that it matches the inclusion pattern and is not explicitly or implicitly excluded). + */ + fun isFileAffectedByCheck(relativePath: String): Boolean = + filePathRegex.matches(relativePath) && !isFileExempted(relativePath) + + /** + * Returns the list of line indexes which contain prohibited content per this check (given an + * iterable of lines). Note that the returned indexes are based on the iteration order of the + * provided iterable. + */ + fun computeAffectedLines(lines: Iterable): List { + return lines.withIndex().filter { (_, line) -> + prohibitedContentRegex.containsMatchIn(line) + }.map { (index, _) -> index } + } + + private fun isFileExempted(relativePath: String): Boolean = + relativePath in exemptedFileNames || exemptedFilePatterns.any { it.matches(relativePath) } + + companion object { + /** Returns a new [MatchableFileContentCheck] based on the specified [FileContentCheck]. */ + fun createFrom(fileContentCheck: FileContentCheck): MatchableFileContentCheck { + return MatchableFileContentCheck( + filePathRegex = fileContentCheck.filePathRegex.toRegex(), + prohibitedContentRegex = fileContentCheck.prohibitedContentRegex.toRegex(), + failureMessage = fileContentCheck.failureMessage, + exemptedFileNames = fileContentCheck.exemptedFileNameList, + exemptedFilePatterns = fileContentCheck.exemptedFilePatternsList.map { it.toRegex() } + ) + } + } +} diff --git a/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt b/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt index e6f8c23870a..675ebe06f53 100644 --- a/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt +++ b/scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt @@ -17,13 +17,18 @@ class RegexPatternValidationCheckTest { private val originalOut: PrintStream = System.out private val REGEX_CHECK_PASSED_OUTPUT_INDICATOR: String = "REGEX PATTERN CHECKS PASSED" private val REGEX_CHECK_FAILED_OUTPUT_INDICATOR: String = "REGEX PATTERN CHECKS FAILED" + private val activitiesPlacementErrorMessage = + "Activities cannot be placed outside the app or testing module." + private val nestedResourceSubdirectoryErrorMessage = + "Only one level of subdirectories under res/ should be maintained (further subdirectories " + + "aren't supported by the project configuration)." private val supportLibraryUsageErrorMessage = "AndroidX should be used instead of the support library" private val coroutineWorkerUsageErrorMessage = "For stable tests, prefer using ListenableWorker with an Oppia-managed dispatcher." private val settableFutureUsageErrorMessage = - "SettableFuture should only be used in pre-approved locations since it's easy to potentially" + - " mess up & lead to a hanging ListenableFuture." + "SettableFuture should only be used in pre-approved locations since it's easy to potentially " + + "mess up & lead to a hanging ListenableFuture." private val androidGravityLeftErrorMessage = "Use android:gravity=\"start\", instead, for proper RTL support" private val androidGravityRightErrorMessage = @@ -47,8 +52,8 @@ class RegexPatternValidationCheckTest { private val androidTouchAnchorSideRightErrorMessage = "Use motion:touchAnchorSide=\"end\", instead, for proper RTL support" private val oppiaCantBeTranslatedErrorMessage = - "Oppia should never used directly in a string (since it shouldn't be translated). Instead," + - " use a parameter & insert the string retrieved from app_name." + "Oppia should never used directly in a string (since it shouldn't be translated). Instead, " + + "use a parameter & insert the string retrieved from app_name." private val untranslatableStringsGoInSpecificFileErrorMessage = "Untranslatable strings should go in untranslated_strings.xml, instead." private val translatableStringsGoInMainFileErrorMessage = @@ -104,8 +109,58 @@ class RegexPatternValidationCheckTest { assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR) assertThat(outContent.toString().trim()).isEqualTo( """ - File name/path violation: Activities cannot be placed outside the app or testing module + File name/path violation: $activitiesPlacementErrorMessage - data/src/main/TestActivity.kt + + $wikiReferenceNote + """.trimIndent() + ) + } + + @Test + fun testFileNamePattern_appResources_stringsFile_fileNamePatternIsCorrect() { + tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values") + tempFolder.newFile("testfiles/app/src/main/res/values/strings.xml") + + runScript() + + assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR) + } + + @Test + fun testFileNamePattern_appResources_subValuesDir_stringsFile_fileNamePatternIsNotCorrect() { + tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values", "subdir") + tempFolder.newFile("testfiles/app/src/main/res/values/subdir/strings.xml") + + val exception = assertThrows(Exception::class) { + runScript() + } + + assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR) + assertThat(outContent.toString().trim()).isEqualTo( + """ + File name/path violation: $nestedResourceSubdirectoryErrorMessage + - app/src/main/res/values/subdir/strings.xml + + $wikiReferenceNote + """.trimIndent() + ) + } + + @Test + fun testFileNamePattern_domainResources_subValuesDir_stringsFile_fileNamePatternIsNotCorrect() { + tempFolder.newFolder("testfiles", "domain", "src", "main", "res", "drawable", "subdir") + tempFolder.newFile("testfiles/domain/src/main/res/drawable/subdir/example.png") + + val exception = assertThrows(Exception::class) { + runScript() + } + + assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR) + assertThat(outContent.toString().trim()).isEqualTo( + """ + File name/path violation: $nestedResourceSubdirectoryErrorMessage + - domain/src/main/res/drawable/subdir/example.png $wikiReferenceNote """.trimIndent() @@ -828,6 +883,30 @@ class RegexPatternValidationCheckTest { assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR) } + @Test + fun testFileContent_translatableString_inPrimaryStringsFile_fileContentIsCorrect() { + val prohibitedContent = "Translatable" + tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values") + val stringFilePath = "app/src/main/res/values/strings.xml" + tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent) + + runScript() + + assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR) + } + + @Test + fun testFileContent_translatableString_inTranslatedPrimaryStringsFile_fileContentIsCorrect() { + val prohibitedContent = "Translatable" + tempFolder.newFolder("testfiles", "app", "src", "main", "res", "values-ar") + val stringFilePath = "app/src/main/res/values-ar/strings.xml" + tempFolder.newFile("testfiles/$stringFilePath").writeText(prohibitedContent) + + runScript() + + assertThat(outContent.toString().trim()).isEqualTo(REGEX_CHECK_PASSED_OUTPUT_INDICATOR) + } + @Test fun testFilenameAndContent_useProhibitedFileName_useProhibitedFileContent_multipleFailures() { tempFolder.newFolder("testfiles", "data", "src", "main") @@ -842,10 +921,10 @@ class RegexPatternValidationCheckTest { assertThat(exception).hasMessageThat().contains(REGEX_CHECK_FAILED_OUTPUT_INDICATOR) assertThat(outContent.toString().trim()).isEqualTo( """ - File name/path violation: Activities cannot be placed outside the app or testing module + File name/path violation: $activitiesPlacementErrorMessage - data/src/main/TestActivity.kt - data/src/main/TestActivity.kt:1: AndroidX should be used instead of the support library + data/src/main/TestActivity.kt:1: $supportLibraryUsageErrorMessage $wikiReferenceNote """.trimIndent() )