[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #452: Schematron Validation

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

mbeckerle commented on a change in pull request #452:
URL: https://github.com/apache/incubator-daffodil/pull/452#discussion_r520028596



##########
File path: daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala
##########
@@ -391,7 +402,7 @@ class CLIConf(arguments: Array[String]) extends scallop.ScallopConf(arguments)
     val threads = opt[Int](short = 't', argName = "threads", default = Some(1), descr = "The number of threads to use.")
     val path = opt[String](argName = "path", descr = "path to the node to create parser.")
     val parser = opt[File](short = 'P', argName = "file", descr = "use a previously saved parser.")
-    val validate: ScallopOption[ValidationMode.Type] = opt[ValidationMode.Type](short = 'V', default = Some(ValidationMode.Off), argName = "mode", descr = "the validation mode. 'on', 'limited' or 'off'.")
+    val validate: ScallopOption[ValidationMode.Type] = opt[ValidationMode.Type](short = 'V', default = Some(ValidationMode.Off), argName = "mode", descr = "the validation mode. 'on', 'limited', 'off', or spi name.")

Review comment:
       Lines too long. Hard to review.
   "spi name" is just going to confuse. How about "validator extension name" or something like that. 

##########
File path: daffodil-lib/src/test/scala/org/apache/daffodil/validation/TestValidatorsSPI.scala
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.validation
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertThrows
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+class TestValidatorsSPI {
+  val schema = getClass.getResource("/test/validation/testSchema1.dfdl.xsd").toURI.toString
+  val infoset = getClass.getResourceAsStream("/test/validation/testData1Infoset.xml")
+
+  @Test def testValidatorGetNotFoundThrows(): Unit = {
+    assertThrows(classOf[ValidatorNotRegisteredException], () => Validators.get("dont exist"))

Review comment:
       Note/FYI: we also have an Intercept implicit that can be used to do this sort of checking. Search tests for "intercept[".
   Honestly I did not even know of assertThrows until now, but it makes sense that it would be in junit. 

##########
File path: daffodil-lib/src/test/scala/org/apache/daffodil/validation/ValidatorSPISupport.scala
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.validation
+
+import java.io.InputStream
+
+import com.typesafe.config.Config
+import org.apache.daffodil.api.ValidationFailure
+import org.apache.daffodil.api.ValidationResult
+import org.apache.daffodil.api.ValidationWarning
+import org.apache.daffodil.api.Validator
+import org.apache.daffodil.api.ValidatorFactory
+
+class PassingValidatorFactory extends ValidatorFactory {
+  def name(): String = PassingValidator.name
+  def make(config: Config): Validator = new TestingValidatorSPI(Seq.empty, Seq.empty)
+}
+object PassingValidator {
+  val name = "passing-validator"
+}
+
+class FailingValidatorFactory extends ValidatorFactory {
+  def name(): String = FailingValidator.name
+  def make(config: Config): Validator = new TestingValidatorSPI(Seq.empty, Seq(ValFail("boom")))

Review comment:
       I'd swear I'm reading this code the second time now. Didn't we already have this above? Or was that Java code?

##########
File path: daffodil-lib/src/test/resources/META-INF/services/org.apache.daffodil.api.ValidatorFactory
##########
@@ -0,0 +1,17 @@
+#  Licensed to the Apache Software Foundation (ASF) under one or more

Review comment:
       Comment about the .keep file above that this interface won't let me put a comment on.
   
   I think we don't need this .keep file. 

##########
File path: daffodil-sapi/src/main/scala/org/apache/daffodil/sapi/Daffodil.scala
##########
@@ -105,6 +106,10 @@ object ValidationMode extends Enumeration {
   val Off = Value(10)
   val Limited = Value(20)
   val Full = Value(30)
+
+  case class Custom(v: Validator) extends ValidationMode {

Review comment:
       After this Validator stuff, we have to figure out how to get rid of the redundancy this API/JAPI/SAPI stuff creates. There is way too much stuff to keep in sync, and way too much duplication, and it gets in the way of easy-extensibility terribly. 

##########
File path: daffodil-schematron/src/test/scala/org/apache/daffodil/validation/ConvertSch.scala
##########
@@ -0,0 +1,14 @@
+package org.apache.daffodil.validation
+//import org.scalatest.{Matchers, WordSpec}
+//
+//
+//class ConvertSch extends WordSpec with Matchers {

Review comment:
       Why commented out?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org