[GitHub] [daffodil] tuxji commented on a change in pull request #601: Distinct return codes for CLI

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

tuxji commented on a change in pull request #601:
URL: https://github.com/apache/daffodil/pull/601#discussion_r663064075



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
     1
   }
 
+  object ExitCode extends Enumeration {
+
+
+
+
+    val Normal = Value(0)

Review comment:
       Yes, ExitCode.Success is more customary.

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
     1
   }
 
+  object ExitCode extends Enumeration {
+
+
+
+
+    val Normal = Value(0)
+    val FileNotFound = Value(1)
+    val InvalidParserException = Value(2)
+    val ExternalVariableException = Value(3)
+    val BindingException = Value(4)
+    val NotYetImplemented = Value(5)
+    val TDMLException = Value(6)
+    val OutOfMemory = Value(7)
+    val UserDefinedFunctionFatalError = Value(8)

Review comment:
       Steve suggested renaming this to UserDefinedFunctionError, agreeing.

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
     1
   }
 
+  object ExitCode extends Enumeration {
+
+
+
+
+    val Normal = Value(0)
+    val FileNotFound = Value(1)
+    val InvalidParserException = Value(2)
+    val ExternalVariableException = Value(3)
+    val BindingException = Value(4)
+    val NotYetImplemented = Value(5)
+    val TDMLException = Value(6)

Review comment:
       Steve suggested renaming this to TestError and regrouping, agreeing.

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1578,47 +1579,81 @@ object Main extends Logging {
     1
   }
 
+  object ExitCode extends Enumeration {
+
+
+
+
+    val Normal = Value(0)
+    val FileNotFound = Value(1)

Review comment:
       Agreed, let's have a catch-all generic ExitCode.Failure (set to 1) in case we want to reduce the number of exit codes later.  Since we're regrouping some errors together, I suggest we start each group of following exit codes from its own new base.  That will give us room to add errors to each group in the future without renumbering following groups of errors.  Feel free to adjust the base of each group to whatever makes sense (I like 8, 16, 32, 64, ... but 10, 20, 30, ... or other bases could work too).

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1451,7 +1452,7 @@ object Main extends Logging {
           println("")
           println("Total: %d, Pass: %d, Fail: %d, Not Found: %s".format(pass + fail + notfound, pass, fail, notfound))
         }
-        0
+        ExitCode.Normal

Review comment:
       I'm fine with returning an ExitCode.TestError if any tests failed to pass.

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -880,7 +880,7 @@ object Main extends Logging {
     }
   }
 
-  def run(arguments: Array[String]): Int = {
+  def run(arguments: Array[String]): ExitCode.Value = {

Review comment:
       That reminded me to grep the entire codebase for exit calls.  We should make sure this PR catches and changes all exit calls.  I found some more calls here:
   
   ```rg
   daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
   120:    System.exit(1)
   304:    sys.exit(1)
   1623:    System.exit(ret)
   
   daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/PropertyGenerator.scala
   703:      System.exit(1);
   
   daffodil-runtime1/src/main/scala/org/apache/daffodil/debugger/InteractiveDebugger.scala
   1723:        sys.exit(1)
   ```

##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -1051,15 +1054,13 @@ object Main extends Logging {
                     val leftOverDataWarning = s"Left over data. Consumed ${loc.bitPos0b} bit(s) with ${remainingBits} bit(s) remaining." + firstByteString + dataHex + dataText
                     log(LogLevel.Warning, leftOverDataWarning)
                     keepParsing = false
-                    error = true
+                    exitCode = ExitCode.LeftOverData

Review comment:
       I'm fine with changing the log level from warning to error.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org