stevedlawrence commented on code in PR #1058:
URL: https://github.com/apache/daffodil/pull/1058#discussion_r1275257544
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/compiler/Compiler.scala:
##########
@@ -110,7 +110,10 @@ 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.argCheck(
Review Comment:
Does this want to be something other than "argCheck"? To me argCheck implies
we are making sure the arg passed in is valid. For example, you might do
something like `argCheck(language != null)`. Feels like we still want this to
be `usage()` but a public API usage variant.
##########
daffodil-japi/src/main/scala/org/apache/daffodil/japi/Daffodil.scala:
##########
@@ -886,3 +887,18 @@ object DaffodilXMLEntityResolver {
DFDLCatalogResolver.get
def getLSResourceResolver: org.w3c.dom.ls.LSResourceResolver =
DFDLCatalogResolver.get
}
+
+/**
+ * Provides functions for assertions for API usage
+ */
+object Assertions {
+
+ /**
+ * Argument check for API functions
+ * @param testFailsIfFalse test that causes failure when false
+ * @param message message to be displayed on failure of assertion
+ */
+ def argCheck(testFailsIfFalse: Boolean, message: String): Unit = {
+ Assert.argCheck(testFailsIfFalse, message)
+ }
+}
Review Comment:
Is this used anywhere? There are no coverage errors but I dont' see where
it's used. Can it be removed if not used?
Note that I think if this function is actually used it will force evaluation
of `message` even if the test passes because it must be passed into this
function. We want to avoid that since there could be some overhead with
generating those strings. The use of macros avoids that.
##########
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:
I think we want to revert this change? The `usage` functions are now about
internal API checks that indicate bugs in Daffoil, so an abort is probably the
right thing to do.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/exceptions/Assert.scala:
##########
@@ -91,6 +91,12 @@ object Assert extends Assert {
def usage(testAbortsIfFalse: Boolean, message: String): Unit = macro
AssertMacros.usageMacro2
def usage(testAbortsIfFalse: Boolean): Unit = macro AssertMacros.usageMacro1
+ /**
+ * Test for the validity of the arguments of the API functions
+ */
+ def argCheck(testThrowsIfFalse: Boolean, message: String): Unit =
Review Comment:
Do we want a name other than "argCheck" for checking public API stuff? The
isError check isn't checking an argument, it's just checking internal state.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala:
##########
@@ -68,7 +68,10 @@ trait SchemaSetRuntime1Mixin {
}.value
def onPath(xpath: String): DFDL.DataProcessor = {
- Assert.usage(!isError)
+ Assert.argCheck(
+ !isError,
+ s"Must check isError before calling onPath. ${getDiagnostics.mkString}",
+ )
Review Comment:
Had a thought, these files are in daffodil-core and so I kind of consider
them as private API. So maybe they still want to call usage()? And then the
onPath/parse/unparse/etc functions in the daffodil-sapi/japi use a public usage
variant that throws an IllegalStateException?
Or maybe these functions are so close to the public API that they are still
considered public?
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -340,8 +340,10 @@ class DataProcessor(
* runtime. Instead we deal with success and failure statuses.
*/
def parse(input: InputSourceDataInputStream, output: InfosetOutputter):
DFDL.ParseResult = {
- Assert.usage(!this.isError)
-
+ Assert.argCheck(
+ !this.isError,
+ s"Must check isError before calling DataProcessor parse.
${this.getDiagnostics.mkString}",
Review Comment:
This error message is making an assumption that the user isn't calling
isError at all. But a user could call isError and have a bug where they just
don't check the result correctly or call parse anyways.. So they might be
confused--"but I am calling isError". Maybe a minor tweak like ?
> DataProcessor parse() must not be called if isError() returns true
It also feels odd to include diagnostics the message. Seems like this could
hide the real issue of bad API usage. This could be more of an issue if there
are alot of warnings mixed in, which might make it very hard to see this one
like about bad API usage. Maybe we could filter the diags and show only the
error? Might be even better to make the one error a `cuase` to the
IllegalArgumentException. But I wonder if we should even do that? A user might
see that diagnostic and fix the schema, when really they need to fix their
code.
--
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]
