Repository: Microsoft/thrifty (kotlin)

Issue:
Kotlin: Union data types can have invalid names
We currently just name sealed type members exactly as the Thrift field, which leads to surprising type names like `TheUnion.foo".  I feel like we should deviate from the precedent set by our configurable field-naming policy, and enforce PascalCase type names.  It's just _too_ weird otherwise.

Pull request:
Fixes #304

Patch:
diff --git a/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt b/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
index 9f1a5751e..c5ba4c288 100644
--- a/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
+++ b/thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt
@@ -532,6 +532,7 @@ class KotlinCodeGenerator(
         val nameAllocator = nameAllocators[struct]
         for (field in struct.fields) {
             val name = nameAllocator.get(field)
+            val sealedName = FieldNamingPolicy.PASCAL.apply(name)
             val type = field.type.typeName
 
             val propertySpec = PropertySpec.builder("value", type)
@@ -540,7 +541,7 @@ class KotlinCodeGenerator(
             val propConstructor = FunSpec.constructorBuilder()
                     .addParameter("value", type)
 
-            val dataProp = TypeSpec.classBuilder(name)
+            val dataProp = TypeSpec.classBuilder(sealedName)
                     .addModifiers(KModifier.DATA)
                     .superclass(structClassName)
                     .addProperty(propertySpec.build())
@@ -555,7 +556,7 @@ class KotlinCodeGenerator(
 
             if (field.defaultValue != null) {
                 check(defaultValueTypeName == null) { "Error in thrifty-schema; unions may not have > 1 default value" }
-                defaultValueTypeName = structClassName.nestedClass(name)
+                defaultValueTypeName = structClassName.nestedClass(sealedName)
             }
         }
 
@@ -805,11 +806,12 @@ class KotlinCodeGenerator(
         for (field in struct.fields) {
             val name = nameAllocator.get(field)
             val type = field.type.typeName
+            val typeName = FieldNamingPolicy.PASCAL.apply(name)
 
             // Add a builder fun
             spec.addFunction(FunSpec.builder(name)
                     .addParameter("value", type)
-                    .addStatement("return apply { this.$builderVarName = ${struct.name}.$name(value) }")
+                    .addStatement("return apply { this.$builderVarName = ${struct.name}.$typeName(value) }")
                     .build())
         }
 
@@ -1031,8 +1033,9 @@ class KotlinCodeGenerator(
         for (field in struct.fields) {
             val name = nameAllocator.get(field)
             val fieldType = field.type
+            val typeName = FieldNamingPolicy.PASCAL.apply(name)
 
-            writer.beginControlFlow("is $name ->")
+            writer.beginControlFlow("is $typeName ->")
 
             writer.addStatement("protocol.writeFieldBegin(%S, %L, %T.%L)",
                     field.name,
@@ -1078,6 +1081,7 @@ class KotlinCodeGenerator(
             for (field in struct.fields) {
                 val name = nameAllocator.get(field)
                 val fieldType = field.type
+                val typeName = FieldNamingPolicy.PASCAL.apply(name)
 
                 reader.addCode {
                     addStatement("${field.id} -> {%>")
@@ -1088,7 +1092,7 @@ class KotlinCodeGenerator(
                     if (builderType != null) {
                         addStatement("builder.$name($name)")
                     } else {
-                        addStatement("result = $name($name)")
+                        addStatement("result = $typeName($name)")
                     }
 
                     nextControlFlow("else")
diff --git a/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicy.kt b/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicy.kt
index 7830509e6..fe5e241ee 100644
--- a/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicy.kt
+++ b/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicy.kt
@@ -81,6 +81,27 @@ abstract class FieldNamingPolicy {
             }
         }
 
+        /**
+         * The Pascal-case policy generates PascalCase names.
+         */
+        val PASCAL: FieldNamingPolicy = object : FieldNamingPolicy() {
+            override fun apply(name: String): String {
+                val caseFormat = caseFormatOf(name)
+                if (caseFormat != null) {
+                    return caseFormat.to(CaseFormat.UPPER_CAMEL, name)
+                }
+
+                // Unknown format.  We'll bulldoze the name by uppercasing the
+                // first char, then just removing any subsequent non-identifier chars.
+                return buildString {
+                    append(Character.toUpperCase(name[0]))
+                    name.substring(1)
+                            .filter { it.isJavaIdentifierPart() }
+                            .forEach { append(it) }
+                }
+            }
+        }
+
         /**
          * Find case format from string.
          * @param s the input String


Test patch:
diff --git a/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt b/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
index 5f822917a..d85ca293d 100644
--- a/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
+++ b/thrifty-kotlin-codegen/src/test/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGeneratorTest.kt
@@ -729,7 +729,7 @@ class KotlinCodeGeneratorTest {
 
         file.single().toString() should contain("""
             |        @JvmStatic
-            |        val DEFAULT: HasDefault = int(16)
+            |        val DEFAULT: HasDefault = Int(16)
         """.trimMargin())
     }
 
diff --git a/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicyTest.kt b/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicyTest.kt
index 914d40d3d..f0e15030c 100644
--- a/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicyTest.kt
+++ b/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/FieldNamingPolicyTest.kt
@@ -62,4 +62,15 @@ class FieldNamingPolicyTest {
         // UPPER_UNDERSCORE
         policy.apply("MY_FIELD") shouldBe "myField"
     }
+
+    @Test
+    fun pascalPolicy() {
+        val policy = FieldNamingPolicy.PASCAL
+        policy.apply("my_field") shouldBe "MyField"
+        policy.apply("my-field") shouldBe "MyField"
+        policy.apply("MyField") shouldBe "MyField"
+        policy.apply("MY_FIELD") shouldBe "MyField"
+        policy.apply("my_f234") shouldBe "MyF234"
+        policy.apply("myField") shouldBe "MyField"
+    }
 }