stevedlawrence commented on code in PR #1058:
URL: https://github.com/apache/daffodil/pull/1058#discussion_r1269923815
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -340,7 +340,7 @@ class DataProcessor(
* runtime. Instead we deal with success and failure statuses.
*/
def parse(input: InputSourceDataInputStream, output: InfosetOutputter):
DFDL.ParseResult = {
- Assert.usage(!this.isError)
+ Assert.usage(!this.isError, this.getDiagnostics.mkString)
Review Comment:
Same comment as above, these message want to be about not calling
parse/unparse when DataProcessor.isError is true.
##########
daffodil-macro-lib/src/main/scala/org/apache/daffodil/lib/exceptions/AssertMacros.scala:
##########
@@ -28,7 +28,7 @@ object AssertMacros {
q"""
if (!($testAbortsIfFalse)) {
- Assert.abort2("Usage error: " + $message, $testAsString)
+ throw new IllegalStateException("Usage error: " + $message +
$testAsString)
Review Comment:
Do we want to remove the testAsString? For an API user, that really isn't
going to mean very much. Instead the message should be descriptive enough to
tell the user how to fix the issue?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/Assert.scala:
##########
@@ -128,7 +128,7 @@ object Assert extends Assert {
//
def usageError(message: String = "Usage error."): Nothing = {
- abort(message)
+ toss(new IllegalStateException(message + "\n" + shortBacktrace))
Review Comment:
Do we need to include shortBacktrace? I think the `IllegalStateException`
should contain a stack trace that API users can output if they want.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala:
##########
@@ -68,7 +68,7 @@ trait SchemaSetRuntime1Mixin {
}.value
def onPath(xpath: String): DFDL.DataProcessor = {
- Assert.usage(!isError)
+ Assert.usage(!isError, getDiagnostics.mkString)
Review Comment:
Same comment as above, but about the `onPath` function instead of
`forLanguage`.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -501,7 +501,7 @@ class DataProcessor(
}
def unparse(inputter: InfosetInputter, output: DFDL.Output):
DFDL.UnparseResult = {
- Assert.usage(!this.isError)
+ Assert.usage(!this.isError, this.getDiagnostics.mkString)
Review Comment:
Grepping through the code, there are a ton of places where use have usage
checks. I *think* all of the places except for the ones in this PR indicate a
bug in Daffodil if they fail. However, the usage checks in this PR indicate a
API user doing something wrong.
So we sort of have two different kinds of usage checks. One where we're
doing internal Daffodil checks and one
doing external API checks. I wonder if the former wants to stay as aborts,
and only these user API ones want to become IllegalStateExceptions? Maybe using
Assert.usage in the places in this PR is wrong and they should be something
else? Or maybe all the internal ones want to be Assert.invariant? Or maybe we
need two different kinds of usage functions, one for internal API and one for
public API checks?
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -110,7 +110,7 @@ final class ProcessorFactory private (
override def onPath(xpath: String): DFDL.DataProcessor = sset.onPath(xpath)
override def forLanguage(language: String): DFDL.CodeGenerator = {
- Assert.usage(!isError)
+ Assert.usage(!isError, getDiagnostics.mkString)
Review Comment:
`Assert.usage` means that the user calling this API used he API incorrectly.
So instead of displaying the diagnostics associated with the failed
ProcessorFactory, I think the error message wants to be about incorrect API
usage. In this case a usage failure means the user did not check `isError`
before calling the `forLanguage` function. So the message probably wants to
instead be something like:
> ProcessorFactory forLanguage() must not be called when isError is true.
I'm not sure I love that, but something along those lines.
--
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]
