[GitHub] markusthoemmes commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore

[GitBox <gi...@apache.org>]

markusthoemmes commented on a change in pull request #3369: Use pureconfig for CouchDbRestStore
URL: https://github.com/apache/incubator-openwhisk/pull/3369#discussion_r171985779
 
 

 ##########
 File path: common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
 ##########
 @@ -21,37 +21,49 @@ import akka.actor.ActorSystem
 import akka.stream.ActorMaterializer
 import spray.json.RootJsonFormat
 import whisk.common.Logging
-import whisk.core.WhiskConfig
+import whisk.core.ConfigKeys
 import whisk.core.entity.DocumentReader
+import pureconfig._
 
 import scala.reflect.ClassTag
 
+case class CouchDbConfig(provider: String,
+                         protocol: String,
+                         host: String,
+                         port: Int,
+                         username: String,
+                         password: String,
+                         databases: Map[String, String]) {
+  assume(Set(protocol, host, username, password).forall(_.nonEmpty), "At least one expected property is missing")
+
+  def getDatabaseName(entityClass: Class[_]): String = {
 
 Review comment:
   This is a style type comment:
   
   I'd propose a different name/API for this method. In the vein of the `ClassTag` used in `makeStore`, we can use `ClassTag` here as well to make a neat call like `databaseFor[WhiskAuth]`. Proposed diff attached. WDYT?
   
   ```diff
   diff --git a/common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala b/common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
   index 7e01c828..c10436b5 100644
   --- a/common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
   +++ b/common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
   @@ -36,10 +36,11 @@ case class CouchDbConfig(provider: String,
                             databases: Map[String, String]) {
      assume(Set(protocol, host, username, password).forall(_.nonEmpty), "At least one expected property is missing")
    
   -  def getDatabaseName(entityClass: Class[_]): String = {
   -    databases.get(entityClass.getSimpleName) match {
   +  def databaseFor[D](implicit tag: ClassTag[D]): String = {
   +    val entityType = tag.runtimeClass.getSimpleName
   +    databases.get(entityType) match {
          case Some(name) => name
   -      case None       => throw new IllegalArgumentException(s"Database name mapping not found for $entityClass")
   +      case None       => throw new IllegalArgumentException(s"Database name mapping not found for $entityType")
        }
      }
    }
   @@ -63,7 +64,7 @@ object CouchDbStoreProvider extends ArtifactStoreProvider {
          dbConfig.port,
          dbConfig.username,
          dbConfig.password,
   -      dbConfig.getDatabaseName(implicitly[ClassTag[D]].runtimeClass),
   +      dbConfig.databaseFor[D],
          useBatching)
      }
    }
   
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services