[GitHub] [daffodil] mbeckerle commented on a change in pull request #592: Added different exit codes to Daffodil command line

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

mbeckerle commented on a change in pull request #592:
URL: https://github.com/apache/daffodil/pull/592#discussion_r653823917



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1584,35 +1584,35 @@ object Main extends Logging {
     } catch {
       case s: scala.util.control.ControlThrowable => throw s
       case e: java.io.FileNotFoundException => {
-        log(LogLevel.Error, "%s", e.getMessage())
+        log(LogLevel.Error, "Missing File Error", e.getMessage())

Review comment:
       You need tests that elicit these messages. I think they would show you that you need "Missing File Error: %s" otherwise there is no place for the e.getMessage() to get substituted into.
   
   But I also think you should compute the message string from the exception object class name. 
   
   So like this:
   ```
   log(LogLevel.Error, "%s %s", Misc.getNameFromClass(e), e.getMessage())
   ```
   The Misc.getNameFromClass(e) is a utility we created for exactly this sort of purpose. It gives you the class name without the package part. 

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1584,35 +1584,35 @@ object Main extends Logging {
     } catch {
       case s: scala.util.control.ControlThrowable => throw s
       case e: java.io.FileNotFoundException => {
-        log(LogLevel.Error, "%s", e.getMessage())
+        log(LogLevel.Error, "Missing File Error", e.getMessage())
         1
       }
       case e: InvalidParserException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "Invalid Parser Error", e.getMessage())
+        2
       }
       case e: ExternalVariableException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "External Variable Error", e.getMessage())
+        3
       }
       case e: BindingException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "Binding Error", e.getMessage())
+        4
       }
       case e: NotYetImplementedException => {
         nyiFound(e)

Review comment:
       nyiFound case also needs to return a unique return code. 

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -685,8 +685,8 @@ object Main extends Logging {
   }
 
   def createProcessorFromSchema(schema: URI, rootNS: Option[RefQName], path: Option[String],
-    tunables: Map[String, String],
-    mode: ValidationMode.Type): Option[DFDL.DataProcessor] = {
+                                tunables: Map[String, String],

Review comment:
       Your IDE made a bunch of whitespace changes I think. 
   They make your change set look much more complex than it should be. 
   

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1584,35 +1584,35 @@ object Main extends Logging {
     } catch {
       case s: scala.util.control.ControlThrowable => throw s
       case e: java.io.FileNotFoundException => {
-        log(LogLevel.Error, "%s", e.getMessage())
+        log(LogLevel.Error, "Missing File Error", e.getMessage())
         1
       }
       case e: InvalidParserException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "Invalid Parser Error", e.getMessage())
+        2
       }
       case e: ExternalVariableException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "External Variable Error", e.getMessage())
+        3
       }
       case e: BindingException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "Binding Error", e.getMessage())
+        4
       }
       case e: NotYetImplementedException => {
         nyiFound(e)
       }
       case e: TDMLException => {
-        log(LogLevel.Error, "%s", e.getMessage())
-        1
+        log(LogLevel.Error, "TDML Error", e.getMessage())
+        5
       }
       case e: OutOfMemoryError => {
         oomError(e)
       }
       case e: UserDefinedFunctionFatalErrorException => {
-        log(LogLevel.Error, "%s", e.getMessage())
+        log(LogLevel.Error, "User Error", e.getMessage())
         e.printStackTrace()
-        1
+        6
       }
       case e: Exception => {
         bugFound(e)

Review comment:
       bugFound case also needs to return a unique return code. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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