stevedlawrence commented on code in PR #1058:
URL: https://github.com/apache/daffodil/pull/1058#discussion_r1270756751
##########
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))
Review Comment:
There are still a lot of uses of the `usageError` function. Should those be
invariants also? And if so, can this function be deleted? It might be nice if
there's one "right" way to check for incorrect usage.
##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
Review Comment:
There are a still a number of places in the TDMLRunner where Assert.usage is
called. I wonder if those want to be invariants if they should never happen? Or
alternatively if they could happen due to a bad TDML file, I wonder if we
should be throwing a TDMLException? I think maybe the rule is something like:
- A check that should never fail unless there is a bug in the TDML Runner:
Assert.invariant
- A check that should only fail if the TDML file is invalid: throw
TDMLException (I think? is there precedence for what to do when we detect
invalid TDML content?)
I'm not sure if there are functions in the TMDL library that could be called
in a way where it's in an illegal state like with the DataProcessor and
ProcessorFactor objects. If we create a Runner without exceptions, all of its
public methods should be safe to call. I don't think we ever return a Runner
object that represents an error and you cant call certain functions like with
DP/PF.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/InfosetInputter.scala:
##########
Review Comment:
On line 165 of this file, there is this line:
```scala
Assert.usage(isInitialized, "Must be initialized")
```
That feels like that wants to be an invariant since it's check that Daffodil
initialized the infoset inputter correctly.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/Assert.scala:
##########
Review Comment:
If the `usage` functions are now for checking public API usage, then we
probably always want a message to be required as a parametr. So it is maybe
worth removing the functions that don't accept a message? For example, maybe
this want's to be removed:
```scala
def usageErrorUnless(testAbortsIfFalse: Boolean): Unit = macro
AssertMacros.usageMacro1
```
We should also add scaladoc to the usage functions to make it clear they are
only for checking public API usage, and that invariants are for checking
internal state/parameters.
Should we also remove the `UsageException` class if we are going to use the
IllegalStateException?
--
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]
