[GitHub] [daffodil] tuxji commented on a diff in pull request #875: Remove all API deprecated methods/classes/objects

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

tuxji commented on code in PR #875:
URL: https://github.com/apache/daffodil/pull/875#discussion_r1024091607


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/externalvars/ExternalVariablesLoader.scala:
##########
@@ -46,7 +46,7 @@ object ExternalVariablesLoader {
 
   def loadVariables(bindings: Seq[Binding], referringContext: ThrowsSDE, vmap: VariableMap): VariableMap = {
     Assert.usage(referringContext != null, "loadVariables expects 'referringContext' to not be null!")
-    VariableUtils.setExternalVariables(vmap, bindings, referringContext)
+    VariableUtils.withExternalVariables(vmap, bindings, referringContext)
     vmap

Review Comment:
   As Steve noticed later on, `VariableUtils.setExternalVariables` was renamed to `VariableUtils.withExternalVariables` but it still mutates the variable map so this renaming change should be reverted.



##########
daffodil-japi/src/test/java/org/apache/daffodil/example/TestJavaAPI.java:
##########
@@ -971,7 +971,7 @@ public void testJavaAPI21() throws IOException, ClassNotFoundException {
     }
 
     @Test
-    public void testJavaAPI22_setExternalVariablesUsingAbstractMap() throws IOException, ClassNotFoundException, ExternalVariableException {
+    public void testJavaAPI22_withExternalVariablesUsingAbstractMap() throws IOException, ClassNotFoundException, ExternalVariableException {

Review Comment:
   Good catch.



##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -630,23 +630,25 @@ object Main {
     }
   }
 
-  def setupDebugOrTrace(proc: HasSetDebugger, conf: CLIConf) = {
-    if (conf.trace() || conf.debug.isDefined) {
-      val runner =
-        if (conf.trace()) {
-          new TraceDebuggerRunner(STDOUT)
-        } else {
-          if (System.console == null) {
-            Logger.log.warn(s"Using --debug on a non-interactive console may result in display issues")
-          }
-          conf.debug() match {
-            case Some(f) => new CLIDebuggerRunner(new File(f), STDIN, STDOUT)
-            case None => new CLIDebuggerRunner(STDIN, STDOUT)
+  def setupDebugOrTrace(proc: DataProcessor, conf: CLIConf): Option[DataProcessor] = {

Review Comment:
   I would have suggested the same change too, thanks, Steve.



##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -117,6 +116,7 @@ import org.apache.daffodil.xml.QName
 import org.apache.daffodil.xml.RefQName
 import org.apache.daffodil.xml.XMLUtils
 import org.apache.daffodil.xml.DFDLCatalogResolver
+// import org.apache.daffodil.processors.HasSetDebugger

Review Comment:
   I would go further and remove the source file defining HasSetDebugger:
   
   daffodil-lib/src/main/scala/org/apache/daffodil/processors/Runtime.scala
   
   Once nothing uses a piece of code anymore, it becomes dead code and should be removed.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/VariableMap1.scala:
##########
@@ -140,7 +140,7 @@ class VariableInstance private (val rd: VariableRuntimeData)
 
 object VariableUtils {
 
-  def setExternalVariables(currentVMap: VariableMap, bindings: Seq[Binding], referringContext: ThrowsSDE) = {
+  def withExternalVariables(currentVMap: VariableMap, bindings: Seq[Binding], referringContext: ThrowsSDE) = {
     bindings.foreach { b => currentVMap.setExtVariable(b.varQName, b.varValue, referringContext) }

Review Comment:
   I checked daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala lines 297, 308, and 332 in the main branch.  They all have similar bodies:
   
   ```scala
        val newBindings = loadExternalVariables(extVars)
        copy(externalVars = newBindings)
   ```
   
   `loadExternalVariables` is defined in the same file and it returns a new data structure, so the `withExternalVariables` methods do not mutate state instead of copying.  There is no need for concern.



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