[GitHub] [daffodil] tuxji commented on a diff in pull request #977: Add WarnID to Schema Definition Warning messages

["tuxji (via GitHub)" <gi...@apache.org>]

tuxji commented on code in PR #977:
URL: https://github.com/apache/daffodil/pull/977#discussion_r1123872866


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dsom/SDE.scala:
##########
@@ -120,10 +121,19 @@ class SchemaDefinitionWarning(
   ) {
 
   def this(sc: SchemaFileLocation, kind: String, args: Any*) =
-    this(Some(sc), None, kind, args: _*)
+    this(None, Some(sc), None, kind, args: _*)

Review Comment:
   Codecov says this line is not covered by tests.  It's possible that this method is no longer called and can be removed safely.  I would go further and remove all the methods and macros which don't take a warn id, especially if there are only one or two callers which we would need to fix.



##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/dsom/SDE.scala:
##########
@@ -120,10 +121,19 @@ class SchemaDefinitionWarning(
   ) {
 
   def this(sc: SchemaFileLocation, kind: String, args: Any*) =
-    this(Some(sc), None, kind, args: _*)
+    this(None, Some(sc), None, kind, args: _*)
 
   override def isError = false
   def modeName = "Schema Definition"
+
+  override def toString = warnID match {
+    case Some(id) =>
+      super.toString + "\n" +
+        s"To suppress this warning, add '${id}' to the " +
+        "daf:suppressSchemaDefinitionWarnings element of a Daffodil defineConfig " +
+        "used when processing"

Review Comment:
   I agree we should be as terse as possible and add only the WarnID to the message.  I suspect it'll be easier to append the WarnID identifier after the format string than to insert it between the Schema Definition Warning and the format string.



##########
daffodil-codegen-c/src/main/scala/org/apache/daffodil/codegen/c/DaffodilCCodeGenerator.scala:
##########
@@ -216,7 +216,7 @@ class DaffodilCCodeGenerator(root: Root) extends DFDL.CodeGenerator {
    * Adds a warning message to the diagnostics
    */
   private def warning(formatString: String, args: Any*): Unit = {
-    val sde = new SchemaDefinitionWarning(None, None, formatString, args: _*)
+    val sde = new SchemaDefinitionWarning(None, None, None, formatString, args: _*)

Review Comment:
   I checked for other SDW calls without a warn id.  We have one more place (I might have missed others):
   
   ```
   daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala
    452   │           pstate.SDW(
    453   │             "Cannot set variable %s after reading the default value. State was: %s. Existing value: %s",
   ```
   
   If we are only two places away from creating all warnings with warning identifiers, then let's create a `WarnID.CodeGeneratorWarning` for this place, a `WarnID.VariableSet` for the second place, and remove all the SDW and schemaDefinition[When|Unless] methods which create warnings without a warn id.
   
   In fact, I also suggest that we make it easier to find all SDW calls in the future by renaming the schemaDefinition[When|Unless] methods/calls to SDWwhen and SDWunless so that *all* SDW calls start with `SDW`.  Does that sound okay?



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