[GitHub] [daffodil] stevedlawrence commented on a change in pull request #771: Fix saveParser CLI with config having tunables.

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

stevedlawrence commented on a change in pull request #771:
URL: https://github.com/apache/daffodil/pull/771#discussion_r824772867



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -609,17 +609,8 @@ object Main {
   def retrieveTunables(tunables: Map[String, String], configFileNode: Option[Node]) = {
     val configFileTunables: Map[String, String] = configFileNode match {
       case None => Map.empty
-      case Some(configNode) => {
-        val tunablesOpt = (configNode \ "tunables").headOption
-        tunablesOpt match {
-          case None => Map.empty
-          case Some(tunableNode) => {
-            tunableNode.child.map { n => (n.label, n.text) }.toMap
-          }
-        }
-      }
+      case Some(configNode) => DaffodilTunables.tunablesMap(configNode)

Review comment:
       I think the TDMLRunner also does some trim stuff with tunables to solve this same problem. Probably want to modify that to use this new code so there's one place that deal with parsing tunables from Scala Noes.
   
   Also, I wonder if it would be helpful to add another overloaded `tunablesMap` function that accepts a `File` as a parameter. Seems like that might reduce duplication that both Main and TDMLRunner need to support, so that neither needs to load the config file into XML first?
   
   I don't know if this is necessary, but I wonder if eventually we want a `DaffodilConfig` class that handles parsing and extracting inforation from config files? Again with the goal of reducing duplication between Main and the TDMLRunner. That way the variables and tunables (and maybe future config stuff) that is stored in a config file can all be handled consistently.

##########
File path: daffodil-propgen/src/main/scala/org/apache/daffodil/propGen/TunableGenerator.scala
##########
@@ -76,21 +76,25 @@ class TunableGenerator(schemaRootConfig: scala.xml.Node, schemaRootExt: scala.xm
     |      if (configOpt.isDefined) {
     |        val loader = new DaffodilXMLLoader()
     |        val node = loader.load(URISchemaSource(configOpt.get), Some(XMLUtils.dafextURI))
-    |        val trimmed = scala.xml.Utility.trim(node)
-    |        val tunablesNode = (trimmed \ "tunables").headOption
-    |        val tunablesMap: Map[String, String] = tunablesNode match {
-    |          case None => Map.empty
-    |          case Some(tunableNode) => {
-    |            tunableNode.child.map { n => (n.label, n.text) }.toMap
-    |          }
-    |        }
-    |        tunablesMap
+    |        tunablesMap(node)
     |      } else {
     |        Map.empty
     |      }
     |
     |    new DaffodilTunables().setTunables(configTunables)
     |  }
+    |
+    |  def tunablesMap(node: scala.xml.Node) = {

Review comment:
       What's our standard for including the type of the return value in a function definition? I personally prefer it since I can immediately know the return value, but maybe that' s not very Scala-y?

##########
File path: daffodil-core/src/test/scala/org/apache/daffodil/general/TestTunables.scala
##########
@@ -97,4 +102,15 @@ class TestTunables {
     assertEquals(true, w4.contains("escapeSchemeRefUndefined"))
   }
 
+  @Test def testConfigFileLoadingTunables(): Unit = {
+    val resource: URI = Misc.getRequiredResource("test/configExample.cfg")

Review comment:
       Same here, does this need to just be "configExample.cfg"?

##########
File path: daffodil-cli/src/test/scala/org/apache/daffodil/TestTunables.scala
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil
+
+import org.apache.daffodil.api.DaffodilTunables
+import org.apache.daffodil.api.URISchemaSource
+import org.apache.daffodil.util.Misc
+import org.apache.daffodil.xml.DaffodilXMLLoader
+import org.apache.daffodil.xml.XMLUtils
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+import java.net.URI
+
+class TestTunables {
+
+  // verifies that DAFFODIL-2650 is fixed.
+  @Test def testTunables1(): Unit = {
+    val resource: URI = Misc.getRequiredResource("test/configExample.cfg")
+    val loader = new DaffodilXMLLoader()
+    val node = loader.load(URISchemaSource(resource), Some(XMLUtils.dafextURI))
+    val tunablesMap = Main.retrieveTunables(Map.empty, Some(node))
+    val tunables = DaffodilTunables(tunablesMap)
+    val warnIDStrings = tunables.suppressSchemaDefinitionWarnings.map{ _.toString }
+    assertEquals(2, warnIDStrings.length)
+    assertTrue(warnIDStrings.contains("facetExplicitLengthOutOfRange"))
+    assertTrue(warnIDStrings.contains("encodingErrorPolicyError"))
+  }
+}

Review comment:
       Any particular reason not to create a CLI test using the normal integration test stuff? This is the only test in daffodil-cli that isn't one of those. Suggest we be consistent and make this an Integration test.

##########
File path: daffodil-cli/src/test/scala/org/apache/daffodil/TestTunables.scala
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil
+
+import org.apache.daffodil.api.DaffodilTunables
+import org.apache.daffodil.api.URISchemaSource
+import org.apache.daffodil.util.Misc
+import org.apache.daffodil.xml.DaffodilXMLLoader
+import org.apache.daffodil.xml.XMLUtils
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+import java.net.URI
+
+class TestTunables {
+
+  // verifies that DAFFODIL-2650 is fixed.
+  @Test def testTunables1(): Unit = {
+    val resource: URI = Misc.getRequiredResource("test/configExample.cfg")

Review comment:
       I think this needs to be just "configExample.cfg"?




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