mbeckerle commented on code in PR #812:
URL: https://github.com/apache/daffodil/pull/812#discussion_r918167848
##########
build.sbt:
##########
@@ -57,6 +57,7 @@ lazy val io = Project("daffodil-io",
file("daffodil-io")).configs(
lazy val runtime1 = Project("daffodil-runtime1",
file("daffodil-runtime1")).configs(IntegrationTest)
.dependsOn(io, lib % "test->test", udf, macroLib
% "compile-internal, test-internal")
.settings(commonSettings, usesMacros)
+ .settings(libraryDependencies ++=
Dependencies.exi)
Review Comment:
This should only be on the CLI. This assumes the elimination of a
daffodil-runtime base class per below which is forcing you to have this here.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DaffodilParseOutputStreamContentHandler.scala:
##########
@@ -309,3 +318,67 @@ class DaffodilParseOutputStreamContentHandler(out:
OutputStream, pretty: Boolean
// do nothing
}
}
+
+/**
+ * ContentHandler implementation that receives SAX events from
DaffodilParseXMLReader to output
+ * EXI to the specified outputStream.
+ *
+ * @param out outputStream object to write generated XML to
+ */
+class DaffodilParseOutputStreamEXIContentHandler(out: OutputStream)
+ extends DaffodilParseOutputStreamContentHandler(out) {
Review Comment:
Even in the Scala API I suggest avoiding dependence on implicit classes.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DaffodilParseOutputStreamContentHandler.scala:
##########
@@ -309,3 +318,67 @@ class DaffodilParseOutputStreamContentHandler(out:
OutputStream, pretty: Boolean
// do nothing
}
}
+
+/**
+ * ContentHandler implementation that receives SAX events from
DaffodilParseXMLReader to output
+ * EXI to the specified outputStream.
+ *
+ * @param out outputStream object to write generated XML to
+ */
+class DaffodilParseOutputStreamEXIContentHandler(out: OutputStream)
+ extends DaffodilParseOutputStreamContentHandler(out) {
Review Comment:
I think the SAX Daffodil code introduced this class in order to add a reset
method so as to have a reset() method on InfosetOutputters, and also on the SAX
ContentHandler, but this requires everything that uses our SAX API to do what
you have done here and create a full proxy object that does almost nothing but
delegate.
In SAX I believe the endDocument call is the one that is supposed to do the
flush, and startDocument the resetting of any state if needed when the object
is reusable, or detect and throw an error if it is not reusable.
So I think reset() is just unnecessary as a method.
If you get rid of reset() then the base class can go away and these become
ordinary content handlers. What reset does today you would do in startDocument
and/or endDocument.
That change will ripple a bit across the API wherever this base class was
used, but it's all good simplifications, and gets us closer to the goal where
Daffodil actually doesn't need modifications to use EXI.
The upshot of this is that what you have in the lines 331-335 below will
move into Main.scala where you currently create this object you would instead
create just the exiContentHandler like at line 335.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -669,7 +674,8 @@ object Main {
case InfosetType.JSON => Left(new JsonInfosetOutputter(os, pretty =
true))
case InfosetType.JDOM => Left(new JDOMInfosetOutputter())
case InfosetType.W3CDOM => Left(new W3CDOMInfosetOutputter())
- case InfosetType.SAX => Right(new
DaffodilParseOutputStreamContentHandler(os, pretty = true))
+ case InfosetType.SAX => Right(new
DaffodilParseOutputStreamXMLContentHandler(os, pretty = true))
+ case InfosetType.EXI => Right(new
DaffodilParseOutputStreamEXIContentHandler(os))
Review Comment:
Per comments below, right here (or in a private method of Main called from
here) is where the code you have in the
DaffodilParseOutputStreamEXIContentHandler constructor would go. That would
allow you to remove the dependency on EXI from the runtime library and put it
only on the CLI library.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/Main.scala:
##########
@@ -669,7 +674,8 @@ object Main {
case InfosetType.JSON => Left(new JsonInfosetOutputter(os, pretty =
true))
case InfosetType.JDOM => Left(new JDOMInfosetOutputter())
case InfosetType.W3CDOM => Left(new W3CDOMInfosetOutputter())
- case InfosetType.SAX => Right(new
DaffodilParseOutputStreamContentHandler(os, pretty = true))
+ case InfosetType.SAX => Right(new
DaffodilParseOutputStreamXMLContentHandler(os, pretty = true))
Review Comment:
(github has decided I cannot comment directly on an unmodified line. Grrrr)
At line 670 above, that really should just be ```Either[InfosetOutputter,
sax.ContentHandler]```
--
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]
