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

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

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


##########
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:
   This feels too verbose to output for every single warning. Especially since there are other ways to define which warnings to suppress, like via the API setting tunables. I'd suggest we just append the id either to the beginning or end of the warning message. Coulple of different ideas:
   
   ```
   Schema Definition Warning [deprecatedBuiltInFormats]: warning message here
   ```
   
   ```
   Schema Definition Warning: warning message here (id: deprecatedBuiltInFormas)
   ```
   
   I don't feel strongly about where it goes, but it should be pretty terse. Diagnostis are already verbose enough with all the file/line number information.



##########
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:
   It would be nice if all warnings had a warn id. I assume this is the only place where we don't have an id? Maybe we can create a single warn ID like `CodeGenerator` for al code generator warnings? Eventually we may want to update this so there are unique warnings and/or have the ability to ignore code generator warnings like we can others.
   
   @tuxji, thoughts?



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

Review Comment:
   I'm not sure toString is the right thing to override. If a user calls `getMessage` instead of `toString` they won't get the warnID. I'm not sure exatly what to override though, we might need to make something in Diagnostic protected. Or alternatively SDW's could add the warn id to the format string?



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