lastland / scala-forklift

Type-safe data migration tool for Slick, Git and beyond.
Other
188 stars 31 forks source link

Don't swallow exceptions silently in `mg new s` #29

Closed arturaz closed 7 years ago

arturaz commented 7 years ago

https://github.com/lastland/scala-forklift/blob/develop/migrations/slick/src/main/scala/Commands.scala#L252

> mg new
[info] Running MyMigrations new
you must enter a proper parameter!

When the real problem was that the application.conf was not parseable. This is very confusing.

arturaz commented 7 years ago

Suggested fix:

  val typeArgs = Map(
    "sql" -> SQL,
    "s" -> SQL,
    "dbio" -> DBIO,
    "d" -> DBIO,
    "api" -> API,
    "a" -> API
  )

  def addMigrationCommand(options: Seq[String]) {
    val firstArg = options.headOption
    val tpeOpt = firstArg.map(_.toLowerCase).flatMap(typeArgs.get)
    tpeOpt match {
      case None =>
        val msg = firstArg.fold(
          "Missing migration type parameter"
        )(a => s"Unknown migration type parameter '$a'")
        val supported = typeArgs.keys.toVector.sorted.mkString("|")
        println(s"$msg: please specify one of the $supported")
      case Some(tpe) =>
        addMigrationOp(tpe, nextId)
        // no need to close db since it's not initialized in this command
    }
  }
lastland commented 7 years ago

Hi @arturaz,

Thanks for the great suggestion! I will take a shot at this today.

lastland commented 7 years ago

Just made a commit to fix this. I will publish the fixed version as 0.2.4-SNAPSHOT as soon as it passed CI tests.

I like your idea of using Option and flatMap, but I find the part where the error message is printed not so readable. That's why I use Try and flatMap instead to pass down the result/exception. (I wonder why Scala doesn't have a built-in Seq.headTry method or a way to convert Option to Try?)

lastland commented 7 years ago

I have published a new snapshot version (0.2.4-SNAPSHOT) which resolves this. Thanks again for reporting this and for the suggestion!

The complete diff can be found here: https://github.com/lastland/scala-forklift/compare/e0724a3...7599dab

PS: I have used custom exceptions instead of println to show error information. Not sure if this would be a good choice. I would like to hear anyone's opinions on this.