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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
