stevedlawrence commented on code in PR #829:
URL: https://github.com/apache/daffodil/pull/829#discussion_r949230926
##########
daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala:
##########
@@ -123,85 +123,85 @@ abstract class InfosetOutputter extends SInfosetOutputter
{
/**
* Called by Daffodil internals to signify the beginning of the infoset.
*
- * @return true on sucess, false if there was an error and Daffodil should
stop all
- * future calls to the InfosetOutputter
+ * @throws Exception if there was an error and Daffodil should stop parsing
*/
- def startDocument(): Boolean
+ @throws[Exception]
Review Comment:
> Very broad throw class. You are basically telling people "don't catch
this".
I feel like it's really saying "if you have any error at all, just throw an
Exception, we don't really care what it is. We'll turn it into an SDE". Maybe
that's not a good API decision though.
Switching to custom exceptions does add extra complexity. InfosetOutputter
implementations are going to do something like this:
```
def startSimple(...): Unit = {
try {
// do someting
} catch {
catch e: SomeExpectedException => throw new InfosetOutputterException(e)
}
}
```
And then our `InfosetWalker` code is going to look like this:
```scala
def doOutputter(outputterFunc: => Unit, ...) {
try {
outputterFunc
} catch {
case e: InfosetOutputterException => context.SDE("infoset outputter
failed: " + e.getCause.toString)
case e: Exception => Assert.usage("Should only throw
InfosetOutputterException")
}
}
```
So we catch the expected exception, unwrap it, and turn it into an SDE. And
any other exception becomes an usage error.
This makes for a cleaner API and detects uncaught exceptions, but I imagine
most implementations are just going to wrap all their logic in giant try/catch
blocks (needed for each of the `InfosetOutputter` functions), wrap that
exception in our custom exception, and then we'll immediately unwrap it and SDE.
This is probably the right approach, but it does adds a lot of extra
boilerplate to InfosetOutputter impelmentations.
It's made even more difficult in Scala since it doesn't warn about uncaught
exceptions. So if our we're missing any try/catches in our built-in
InfosetOutputters, we aren't going to know until a usage error pops up. Maybe
that's what we want though?
##########
daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala:
##########
@@ -123,85 +123,85 @@ abstract class InfosetOutputter extends SInfosetOutputter
{
/**
* Called by Daffodil internals to signify the beginning of the infoset.
*
- * @return true on sucess, false if there was an error and Daffodil should
stop all
- * future calls to the InfosetOutputter
+ * @throws Exception if there was an error and Daffodil should stop parsing
*/
- def startDocument(): Boolean
+ @throws[Exception]
Review Comment:
> Very broad throw class. You are basically telling people "don't catch
this".
I feel like it's really saying "if you have any error at all, just throw an
Exception, we don't really care what it is. We'll turn it into an SDE". Maybe
that's not a good API decision though.
Switching to custom exceptions does add extra complexity. InfosetOutputter
implementations are going to do something like this:
```scala
def startSimple(...): Unit = {
try {
// do someting
} catch {
catch e: SomeExpectedException => throw new InfosetOutputterException(e)
}
}
```
And then our `InfosetWalker` code is going to look like this:
```scala
def doOutputter(outputterFunc: => Unit, ...) {
try {
outputterFunc
} catch {
case e: InfosetOutputterException => context.SDE("infoset outputter
failed: " + e.getCause.toString)
case e: Exception => Assert.usage("Should only throw
InfosetOutputterException")
}
}
```
So we catch the expected exception, unwrap it, and turn it into an SDE. And
any other exception becomes an usage error.
This makes for a cleaner API and detects uncaught exceptions, but I imagine
most implementations are just going to wrap all their logic in giant try/catch
blocks (needed for each of the `InfosetOutputter` functions), wrap that
exception in our custom exception, and then we'll immediately unwrap it and SDE.
This is probably the right approach, but it does adds a lot of extra
boilerplate to InfosetOutputter impelmentations.
It's made even more difficult in Scala since it doesn't warn about uncaught
exceptions. So if our we're missing any try/catches in our built-in
InfosetOutputters, we aren't going to know until a usage error pops up. Maybe
that's what we want though?
--
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]
