stevedlawrence commented on code in PR #875:
URL: https://github.com/apache/daffodil/pull/875#discussion_r1023962258
##########
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:
Remove comment
##########
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:
Instead of returning an option, I would just suggest returning an unmutated
proc, e.g.
```scala
(conf.trace() || conf.debug.isDefined) match {
case true => {
...
proc.withDebugger(id).withDebugging(true)
}
case false => proc
}
```
Then callers just do:
```scala
val debugProcessor = setupDebugOrTrace(proc, conf)
```
##########
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:
Does `withExternalVariable` mutate the vmap, or does it create a copy? I
assume it creates a copy with the new values, so I would expect this to be
something like:
```scala
val newVMap = VariableUtils.withExernalVariables(...)
newVMap
```
##########
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:
This calls `setExternalVariable` which looks like it mutates the variable
map. Because of that this should probably stay as `setExternalVariables` to
make it clear this mutates state.
This also related to another comment I made about what calls this function,
that comment probably no longer applies.
Note I wonder if there is a bug...do our API `withExternalVariables`
functions in the DataProcessor end up calling this `setExternalVariables`
function and mutate state instead of copying? We may need to investigate that,
but that can be done separately.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/XMLTextInfosetInputter.scala:
##########
@@ -191,9 +191,6 @@ object XMLTextInfoset {
class XMLTextInfosetInputter private (input: Either[java.io.Reader,
java.io.InputStream])
Review Comment:
Can this remove the `Either`? Seems like we only need the `InputStream` now?
##########
daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala:
##########
@@ -91,11 +91,11 @@ class TunableGenerator(schemaRootConfig: scala.xml.Node,
schemaRootExt: scala.xm
val middle = """
| extends Serializable {
|
- | def setTunables(tunables: Map[String, String]): DaffodilTunables = {
- | tunables.foldLeft(this) { case (dafTuns, (tunable, value)) =>
dafTuns.setTunable(tunable, value) }
+ | def withTunables(tunables: Map[String, String]): DaffodilTunables = {
+ | tunables.foldLeft(this) { case (dafTuns, (tunable, value)) =>
dafTuns.withTunable(tunable, value) }
| }
|
- | def setTunable(tunable: String, value: String): DaffodilTunables = {
+ | def withTunable(tunable: String, value: String): DaffodilTunables = {
Review Comment:
This functions aren't technically deprecated, but this rename is
semi-related to other deprecated functions and is a good change--the generated
`setTunable` functions are immutable and so should use the same `with*` naming
convention :+1:
--
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]
