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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
