Deprecate functions with 'choice' arguments (#245)

in favor of "plural" and "select" arguments
cashapp · Oct 8, 2023 · 170152f · 170152f
1 parent fcd17c2
commit 170152f
Show file tree

Hide file tree

Showing 7 changed files with 123 additions and 6 deletions.
diff --git a/plugin/src/main/java/app/cash/paraphrase/plugin/ResourceMerger.kt b/plugin/src/main/java/app/cash/paraphrase/plugin/ResourceMerger.kt
@@ -49,6 +49,17 @@ internal fun mergeResources(
     (0 until argumentCount).toSet() == tokenNumbers
   }
 
+  val deprecation = if (defaultResource.tokens.any { it.type == TokenType.Choice }) {
+    MergedResource.Deprecation.WithMessage(
+      message = """
+        Use of the old 'choice' argument type is discouraged. Use a 'plural' argument to select
+        sub-messages based on a numeric value, together with the plural rules for the specified
+        language. Use a 'select' argument to select sub-messages via a fixed set of keywords.
+      """.trimIndent().replace("\n", " "),
+    )
+  } else {
+    MergedResource.Deprecation.None
+  }
   val arguments = defaultResource.tokens
     .groupBy { it.argumentKey }
     .mapValues { (argumentKey, tokens) ->
@@ -66,6 +77,7 @@ internal fun mergeResources(
     description = defaultResource.description,
     visibility = publicResources.resolveVisibility(name = name, type = "string"),
     arguments = arguments.values.filterNotNull(),
+    deprecation = deprecation,
     hasContiguousNumberedTokens = hasContiguousNumberedTokens,
     parsingErrors = arguments.filterValues { it == null }.keys.map {
       "Incompatible argument types for: $it"

diff --git a/plugin/src/main/java/app/cash/paraphrase/plugin/ResourceWriter.kt b/plugin/src/main/java/app/cash/paraphrase/plugin/ResourceWriter.kt
@@ -17,7 +17,9 @@ package app.cash.paraphrase.plugin
 
 import app.cash.paraphrase.plugin.model.MergedResource
 import app.cash.paraphrase.plugin.model.MergedResource.Argument
+import app.cash.paraphrase.plugin.model.MergedResource.Deprecation
 import com.squareup.kotlinpoet.ANY
+import com.squareup.kotlinpoet.AnnotationSpec
 import com.squareup.kotlinpoet.ClassName
 import com.squareup.kotlinpoet.CodeBlock
 import com.squareup.kotlinpoet.FileSpec
@@ -83,6 +85,10 @@ private fun MergedResource.toFunSpec(packageStringsType: TypeName): FunSpec {
     .apply { arguments.forEach { addParameter(it.toParameterSpec()) } }
     .returns(Types.FormattedResource)
     .apply {
+      if (deprecation is Deprecation.WithMessage) {
+        addAnnotation(annotationSpec = deprecatedAnnotationSpec(deprecation))
+      }
+
       if (hasContiguousNumberedTokens) {
         addCode(
           buildCodeBlock {
@@ -121,6 +127,12 @@ private fun MergedResource.toFunSpec(packageStringsType: TypeName): FunSpec {
     .build()
 }
 
+private fun deprecatedAnnotationSpec(deprecation: Deprecation.WithMessage): AnnotationSpec {
+  return AnnotationSpec.builder(Deprecated::class)
+    .addMember("%S", deprecation.message)
+    .build()
+}
+
 private fun Argument.toParameterSpec(): ParameterSpec =
   ParameterSpec(
     name = name,

diff --git a/plugin/src/main/java/app/cash/paraphrase/plugin/model/MergedResource.kt b/plugin/src/main/java/app/cash/paraphrase/plugin/model/MergedResource.kt
@@ -25,6 +25,7 @@ internal data class MergedResource(
   val description: String?,
   val visibility: Visibility,
   val arguments: List<Argument>,
+  val deprecation: Deprecation,
   /* True when the arguments bind to a contiguous set of integer tokens counting from 0. */
   val hasContiguousNumberedTokens: Boolean,
   val parsingErrors: List<String>,
@@ -41,4 +42,11 @@ internal data class MergedResource(
     Private,
     Public,
   }
+
+  sealed interface Deprecation {
+    data object None : Deprecation
+    data class WithMessage(
+      val message: String,
+    ) : Deprecation
+  }
 }
diff --git a/plugin/src/test/java/app/cash/paraphrase/plugin/ResourceMergerTest.kt b/plugin/src/test/java/app/cash/paraphrase/plugin/ResourceMergerTest.kt
@@ -15,15 +15,18 @@
  */
 package app.cash.paraphrase.plugin
 
+import app.cash.paraphrase.plugin.TokenType.Choice
 import app.cash.paraphrase.plugin.TokenType.Date
 import app.cash.paraphrase.plugin.TokenType.Plural
 import app.cash.paraphrase.plugin.TokenType.Time
 import app.cash.paraphrase.plugin.model.MergedResource
 import app.cash.paraphrase.plugin.model.MergedResource.Argument
+import app.cash.paraphrase.plugin.model.MergedResource.Deprecation
 import app.cash.paraphrase.plugin.model.PublicResource
 import app.cash.paraphrase.plugin.model.ResourceFolder
 import app.cash.paraphrase.plugin.model.ResourceName
 import app.cash.paraphrase.plugin.model.TokenizedResource
+import app.cash.paraphrase.plugin.model.TokenizedResource.Token.NamedToken
 import app.cash.paraphrase.plugin.model.TokenizedResource.Token.NumberedToken
 import com.google.common.truth.Truth.assertThat
 import java.time.LocalDateTime
@@ -46,6 +49,7 @@ class ResourceMergerTest {
       publicResources = emptyList(),
     )
     assertThat(result!!.visibility).isEqualTo(MergedResource.Visibility.Public)
+    assertThat(result.deprecation).isEqualTo(Deprecation.None)
   }
 
   @Test
@@ -68,6 +72,7 @@ class ResourceMergerTest {
       ),
     )
     assertThat(result!!.visibility).isEqualTo(MergedResource.Visibility.Public)
+    assertThat(result.deprecation).isEqualTo(Deprecation.None)
   }
 
   @Test
@@ -90,6 +95,7 @@ class ResourceMergerTest {
       ),
     )
     assertThat(result!!.visibility).isEqualTo(MergedResource.Visibility.Private)
+    assertThat(result.deprecation).isEqualTo(Deprecation.None)
   }
 
   @Test
@@ -113,6 +119,7 @@ class ResourceMergerTest {
       ),
     )
     assertThat(result!!.visibility).isEqualTo(MergedResource.Visibility.Private)
+    assertThat(result.deprecation).isEqualTo(Deprecation.None)
   }
 
   @Test
@@ -140,6 +147,7 @@ class ResourceMergerTest {
       ),
     )
     assertThat(result.parsingErrors).isEmpty()
+    assertThat(result.deprecation).isEqualTo(Deprecation.None)
   }
 
   @Test
@@ -161,5 +169,29 @@ class ResourceMergerTest {
     )
     assertThat(result!!.arguments).isEmpty()
     assertThat(result.parsingErrors).containsExactly("Incompatible argument types for: 0")
+    assertThat(result.deprecation).isEqualTo(Deprecation.None)
+  }
+
+  @Test
+  fun choiceArgumentProducesDeprecatedFunction() {
+    val result = mergeResources(
+      name = ResourceName("test"),
+      tokenizedResources = mapOf(
+        ResourceFolder.Default to TokenizedResource(
+          name = ResourceName("test"),
+          description = null,
+          tokens = listOf(
+            NamedToken(name = "choice", type = Choice),
+            NamedToken(name = "other", type = Date),
+          ),
+          parsingError = null,
+        ),
+      ),
+      publicResources = emptyList(),
+    )
+
+    val deprecation = result!!.deprecation as Deprecation.WithMessage
+    assertThat(deprecation.message)
+      .contains("Use of the old 'choice' argument type is discouraged")
   }
 }
diff --git a/plugin/src/test/java/app/cash/paraphrase/plugin/ResourceWriterTest.kt b/plugin/src/test/java/app/cash/paraphrase/plugin/ResourceWriterTest.kt
@@ -16,8 +16,10 @@
 package app.cash.paraphrase.plugin
 
 import app.cash.paraphrase.plugin.model.MergedResource
+import app.cash.paraphrase.plugin.model.MergedResource.Deprecation
 import app.cash.paraphrase.plugin.model.ResourceName
 import com.google.common.truth.Truth.assertThat
+import com.squareup.kotlinpoet.AnnotationSpec
 import com.squareup.kotlinpoet.FileSpec
 import com.squareup.kotlinpoet.KModifier
 import com.squareup.kotlinpoet.TypeSpec
@@ -36,6 +38,7 @@ class ResourceWriterTest {
           description = null,
           visibility = MergedResource.Visibility.Public,
           arguments = emptyList(),
+          deprecation = Deprecation.None,
           hasContiguousNumberedTokens = false,
           parsingErrors = emptyList(),
         ),
@@ -58,6 +61,7 @@ class ResourceWriterTest {
           description = null,
           visibility = MergedResource.Visibility.Public,
           arguments = emptyList(),
+          deprecation = Deprecation.None,
           hasContiguousNumberedTokens = false,
           parsingErrors = emptyList(),
         ),
@@ -66,6 +70,7 @@ class ResourceWriterTest {
           description = null,
           visibility = MergedResource.Visibility.Private,
           arguments = emptyList(),
+          deprecation = Deprecation.None,
           hasContiguousNumberedTokens = false,
           parsingErrors = emptyList(),
         ),
@@ -89,6 +94,7 @@ class ResourceWriterTest {
           description = null,
           visibility = MergedResource.Visibility.Private,
           arguments = emptyList(),
+          deprecation = Deprecation.None,
           hasContiguousNumberedTokens = false,
           parsingErrors = emptyList(),
         ),
@@ -97,6 +103,7 @@ class ResourceWriterTest {
           description = null,
           visibility = MergedResource.Visibility.Private,
           arguments = emptyList(),
+          deprecation = Deprecation.None,
           hasContiguousNumberedTokens = false,
           parsingErrors = emptyList(),
         ),
@@ -114,12 +121,7 @@ class ResourceWriterTest {
     expectedClassVisibility: KModifier,
     vararg expectedFunctionVisibility: Pair<String, KModifier>,
   ) {
-    val formattedResourcesObject = members
-      .filterIsInstance<TypeSpec>()
-      .find { it.name == "FormattedResources" }
-    if (formattedResourcesObject == null) {
-      fail("FormattedResources object not found")
-    } else {
+    assertOnFormattedResourcesObject { formattedResourcesObject ->
       assertThat(formattedResourcesObject.modifiers).contains(expectedClassVisibility)
 
       expectedFunctionVisibility.forEach { (name, expectedVisibility) ->
@@ -132,4 +134,44 @@ class ResourceWriterTest {
       }
     }
   }
+
+  @Test
+  fun deprecationWithMessageProducesDeprecationWithMessage() {
+    val result = writeResources(
+      packageName = "com.example",
+      mergedResources = listOf(
+        MergedResource(
+          name = ResourceName("testFun"),
+          description = null,
+          visibility = MergedResource.Visibility.Public,
+          arguments = emptyList(),
+          deprecation = Deprecation.WithMessage("Test message"),
+          hasContiguousNumberedTokens = false,
+          parsingErrors = emptyList(),
+        ),
+      ),
+    )
+
+    result.assertOnFormattedResourcesObject { formattedResourcesObject ->
+      val testFun = formattedResourcesObject.funSpecs.single { it.name == "testFun" }
+      assertThat(testFun.annotations).contains(
+        AnnotationSpec.builder(Deprecated::class)
+          .addMember("%S", "Test message")
+          .build(),
+      )
+    }
+  }
+
+  private inline fun FileSpec.assertOnFormattedResourcesObject(
+    block: (formattedResourcesObject: TypeSpec) -> Unit,
+  ) {
+    val formattedResourcesObject = members
+      .filterIsInstance<TypeSpec>()
+      .find { it.name == "FormattedResources" }
+    if (formattedResourcesObject == null) {
+      fail("FormattedResources object not found")
+    } else {
+      block(formattedResourcesObject)
+    }
+  }
 }
diff --git a/sample/app/src/main/java/app/cash/paraphrase/sample/app/MainActivity.kt b/sample/app/src/main/java/app/cash/paraphrase/sample/app/MainActivity.kt
@@ -143,6 +143,10 @@ class MainActivity : ComponentActivity() {
         label = "Select Ordinal Argument",
         resource = LibraryFormattedResources.library_select_ordinal_argument(count = 5),
       ),
+      Sample(
+        label = "Choice argument",
+        resource = LibraryFormattedResources.library_choice_argument(outlook = 100),
+      ),
     )
   }
 }
diff --git a/sample/library/src/main/res/values/strings.xml b/sample/library/src/main/res/values/strings.xml
@@ -25,4 +25,11 @@
       other {Evelyn verse jumps for the #th time}
     }
   </string>
+  <string name="library_choice_argument">
+    Jobu\'s outlook is {outlook, choice,
+      -1#negative|
+      0#neutral|
+      1#positive
+    }
+  </string>
 </resources>