You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2022/03/10 23:03:19 UTC
[GitHub] [daffodil] mbeckerle opened a new pull request #771: Fix saveParser CLI with config having tunables.
mbeckerle opened a new pull request #771:
URL: https://github.com/apache/daffodil/pull/771
The CLI's loading of config files was not
trimming the node, hence, getting PCDATA nodes
instead of the strings it expected.
Shares code from DaffodilTunables object now.
DAFFODIL-2650
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #771: Fix saveParser CLI with config having tunables.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #771:
URL: https://github.com/apache/daffodil/pull/771#discussion_r824258548
##########
File path: daffodil-cli/src/test/resources/configExample.cfg
##########
@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
Review comment:
Missing Apache banner. Fails rat check.
##########
File path: daffodil-core/src/test/resources/test/configExample.cfg
##########
@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
Review comment:
fails rat check.
--
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
[GitHub] [daffodil] mbeckerle merged pull request #771: Fix saveParser CLI with config having tunables.
Posted by GitBox <gi...@apache.org>.
mbeckerle merged pull request #771:
URL: https://github.com/apache/daffodil/pull/771
--
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
[GitHub] [daffodil] tuxji commented on a change in pull request #771: Fix saveParser CLI with config having tunables.
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #771:
URL: https://github.com/apache/daffodil/pull/771#discussion_r824772747
##########
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")
+ val loader = new DaffodilXMLLoader()
+ val node = loader.load(URISchemaSource(resource), Some(XMLUtils.dafextURI))
+ val tunablesMap = DaffodilTunables.tunablesMap(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:
I am not certain that `testConfigFileLoadingTunables` tests anything different than what `testTunables1` tests in daffodil-cli/src/test/scala/org/apache/daffodil/TestTunables.scala. The purpose of `testTunables1` is to confirm that `Main.retrieveTunables` now does the same thing as `DaffodilTunables.tunablesMap(node)`, even though that's because `Main.retrieveTunables` was changed to call `DaffodilTunables.tunablesMap(node)` itself. Why do we need `testConfigFileLoadingTunables` when it tests a subset of the same code as `testTunables1`? Actually, maybe we simply need to move `Main.retrieveTunables` to `DaffodilTunables.retrieveTunables` and make `testConfigFileLoadingTunables` call it; then we can get rid of `testTunables1`.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #771: Fix saveParser CLI with config having tunables.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #771:
URL: https://github.com/apache/daffodil/pull/771#discussion_r824869394
##########
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:
It's public method, so I think should have the return type.
--
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
[GitHub] [daffodil] stevedlawrence commented on a change in pull request #771: Fix saveParser CLI with config having tunables.
Posted by 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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #771: Fix saveParser CLI with config having tunables.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #771:
URL: https://github.com/apache/daffodil/pull/771#discussion_r825176903
##########
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")
+ val loader = new DaffodilXMLLoader()
+ val node = loader.load(URISchemaSource(resource), Some(XMLUtils.dafextURI))
+ val tunablesMap = DaffodilTunables.tunablesMap(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:
refactored to address this and SL's modularity concerns.
--
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
[GitHub] [daffodil] mbeckerle commented on a change in pull request #771: Fix saveParser CLI with config having tunables.
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #771:
URL: https://github.com/apache/daffodil/pull/771#discussion_r825176622
##########
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 I've addressed this now.
--
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