Merge pull request #1113 from square/ray/invert-keyFor

Improves legibility of `Compatible.keyFor`, `NamedScreen`, `Named`.
square · Oct 5, 2023 · 0a14ba0 · 0a14ba0
2 parents 5b3c774 + db33461
commit 0a14ba0
Show file tree

Hide file tree

Showing 5 changed files with 13 additions and 8 deletions.
diff --git a/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/Compatible.kt b/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/Compatible.kt
@@ -36,14 +36,19 @@ public interface Compatible {
 
   public companion object {
     /**
-     * Calculates a suitable [Compatible.compatibilityKey] for a given [value] and [name].
+     * Calculates a suitable [Compatible.compatibilityKey] for a given [value], incorporating
+     * [name] if that is not blank. Includes the [compatibilityKey] for [value] if it
+     * implements [Compatible], to support recursion from wrapping.
+     *
+     * Style note: [name] is given more prominence than the key generate
      */
     public fun keyFor(
       value: Any,
       name: String = ""
     ): String {
-      return ((value as? Compatible)?.compatibilityKey ?: value::class.java.name) +
-        if (name.isEmpty()) "" else "+$name"
+      val key = (value as? Compatible)?.compatibilityKey ?: value::class.java.name
+
+      return name.takeIf { it.isNotEmpty() }?.let { "$name($key)" } ?: key
     }
   }
 }
diff --git a/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/Named.kt b/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/Named.kt
@@ -15,7 +15,7 @@ public data class Named<W : Any>(
     require(name.isNotBlank()) { "name must not be blank." }
   }
 
-  override val compatibilityKey: String = Compatible.keyFor(wrapped, "Named($name)")
+  override val compatibilityKey: String = Compatible.keyFor(wrapped, "Named:$name")
 
   override fun toString(): String {
     return "${super.toString()}: $compatibilityKey"

diff --git a/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/NamedScreen.kt b/workflow-ui/core-common/src/main/java/com/squareup/workflow1/ui/NamedScreen.kt
@@ -16,7 +16,7 @@ public data class NamedScreen<C : Screen>(
     require(name.isNotBlank()) { "name must not be blank." }
   }
 
-  override val compatibilityKey: String = Compatible.keyFor(content, "NamedScreen($name)")
+  override val compatibilityKey: String = Compatible.keyFor(content, "NamedScreen:$name")
 
   @Deprecated("Use content", ReplaceWith("content"))
   public val wrapped: C = content

diff --git a/workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/NamedScreenTest.kt b/workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/NamedScreenTest.kt
@@ -57,7 +57,7 @@ internal class NamedScreenTest {
 
   @Test fun `recursive keys are legible`() {
     assertThat(NamedScreen(NamedScreen(Hey, "one"), "ho").compatibilityKey)
-      .isEqualTo("com.squareup.workflow1.ui.NamedScreenTest\$Hey+NamedScreen(one)+NamedScreen(ho)")
+      .isEqualTo("NamedScreen:ho(NamedScreen:one(com.squareup.workflow1.ui.NamedScreenTest\$Hey))")
   }
 
   private class Foo(override val compatibilityKey: String) : Compatible, Screen

diff --git a/workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/NamedTest.kt b/workflow-ui/core-common/src/test/java/com/squareup/workflow1/ui/NamedTest.kt
@@ -5,7 +5,7 @@ import org.junit.Test
 
 // If you try to replace isTrue() with isTrue compilation fails.
 @OptIn(WorkflowUiExperimentalApi::class)
-@Suppress("UsePropertyAccessSyntax", "DEPRECATION")
+@Suppress("DEPRECATION")
 internal class NamedTest {
   object Whut
   object Hey
@@ -58,7 +58,7 @@ internal class NamedTest {
 
   @Test fun `recursive keys are legible`() {
     assertThat(Named(Named(Hey, "one"), "ho").compatibilityKey)
-      .isEqualTo("com.squareup.workflow1.ui.NamedTest\$Hey+Named(one)+Named(ho)")
+      .isEqualTo("Named:ho(Named:one(com.squareup.workflow1.ui.NamedTest\$Hey))")
   }
 
   private class Foo(override val compatibilityKey: String) : Compatible