Merge pull request #1534 from twitter/ianoc/fixScroogeMacroHandling

Because, because... fun, the scala compiler has special naming rules …
twitter · Mar 4, 2016 · e7395b3 · e7395b3
2 parents 68b9912 + 04798f3
commit e7395b3
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 6 deletions.
diff --git a/scalding-thrift-macros-fixtures/src/test/resources/test.thrift b/scalding-thrift-macros-fixtures/src/test/resources/test.thrift
@@ -78,11 +78,28 @@ struct TestTypes{
  9: required binary a_binary
 }
 
+struct TestOptionTypes{
+ 1: optional bool a_bool,
+ 2: optional byte a_byte,
+ 3: optional i16 a_i16,
+ 4: optional i32 a_i32,
+ 5: optional i64 a_i64,
+ 6: optional double a_double,
+ 7: optional string a_string,
+ 8: optional TestEnum a_enum,
+ 9: optional binary a_binary
+}
+
 union TestUnion{
  1:  list<i32> a_i32_list,
  2:  set<double> a_double_set,
  3:  TestStruct a_struct
 }
 
+struct A {
+  1: optional string _a
+  7: optional i64 g
+  8: optional i64 abcdEE
+}
 
 
diff --git a/...ala/com/twitter/scalding/thrift/macros/impl/ordered_serialization/ScroogeOrderedBuf.scala b/...ala/com/twitter/scalding/thrift/macros/impl/ordered_serialization/ScroogeOrderedBuf.scala
@@ -49,20 +49,19 @@ object ScroogeOrderedBuf {
     val fieldNames: List[String] = companionSymbol.asModule.moduleClass.asType.toType
       .declarations
       .filter(_.name.decoded.endsWith("Field "))
-      .collect{ case s: TermSymbol => s }
+      .collect { case s: TermSymbol => s }
       .filter(_.isStatic)
       .filter(_.isVal)
       .map { t =>
         val decodedName = t.name.decoded // Looks like "MethodNameField "
-        val cased = decodedName.take(1).toLowerCase ++ decodedName.drop(1) // "methodNameField "
-        cased.dropRight(6) //  (6 == "Field ".length) --> Drop the "Field " from the end to give methodName
+        decodedName.dropRight(6).toLowerCase //  These things end in "Field " , yes there is a space in there
       }.toList
 
     val elementData: List[(c.universe.Type, TermName, TreeOrderedBuf[c.type])] =
       outerType
         .declarations
         .collect { case m: MethodSymbol => m }
-        .filter(m => fieldNames.contains(m.name.toTermName.toString))
+        .filter(m => fieldNames.contains(m.name.toTermName.toString.toLowerCase))
         .map { accessorMethod =>
           val fieldType = accessorMethod.returnType.asSeenFrom(outerType, outerType.typeSymbol.asClass)
           val b: TreeOrderedBuf[c.type] = dispatcher(fieldType)
@@ -101,7 +100,7 @@ object ScroogeOrderedBuf {
         ProductLike.compare(c)(elementA, elementB)(elementData)
 
       override val lazyOuterVariables: Map[String, ctx.Tree] =
-        elementData.map(_._3.lazyOuterVariables).reduce(_ ++ _)
+        elementData.map(_._3.lazyOuterVariables).reduceLeftOption(_ ++ _).getOrElse(Map())
 
       override def length(element: Tree) =
         ProductLike.length(c)(element)(elementData)

diff --git a/...ift-macros/src/test/scala/com/twitter/scalding/thrift/macros/ScroogeMacrosUnitTests.scala b/...ift-macros/src/test/scala/com/twitter/scalding/thrift/macros/ScroogeMacrosUnitTests.scala
@@ -16,7 +16,7 @@
 package com.twitter.scalding.thrift.macros
 
 import com.twitter.scalding.serialization.OrderedSerialization
-import com.twitter.scalding.thrift.macros.scalathrift.TestLists
+import com.twitter.scalding.thrift.macros.scalathrift._
 import org.scalatest.prop.PropertyChecks
 import org.scalatest.{ Matchers, WordSpec }
 
@@ -28,6 +28,15 @@ class ScroogeMacrosUnitTests extends WordSpec with Matchers with PropertyChecks
   import Macros._
 
   "MacroGenerated TBaseOrderedSerialization" should {
+
+    "Should generate serializers" in {
+      Macros.scroogeOrdSer[TestTypes]
+      Macros.scroogeOrdSer[TestLists]
+      Macros.scroogeOrdSer[TestMaps]
+      Macros.scroogeOrdSer[TestOptionTypes]
+      Macros.scroogeOrdSer[A]
+    }
+
     "Should RT" in {
       forAll { a1: TestLists =>
         assert(oBufCompare(rt(a1), a1) == 0)