stevedlawrence commented on code in PR #821:
URL: https://github.com/apache/daffodil/pull/821#discussion_r1042473047
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/XMLTextInfosetOutputter.scala:
##########
@@ -25,18 +25,23 @@ import org.apache.daffodil.dpath.NodeInfo
import org.apache.daffodil.exceptions.Assert
import org.apache.daffodil.util.Indentable
+import org.apache.daffodil.api.XMLOutputStyle
+
+class NoMatchException(s: String) extends Exception(s) {}
+
/**
* Writes the infoset to a java.io.Writer as XML text.
*
* @param writer The writer to write the XML text to
* @param pretty Whether or to enable pretty printing. Set to true, XML
* elements are indented and newlines are inserted.
*/
-class XMLTextInfosetOutputter private (writer: java.io.Writer, pretty: Boolean)
+class XMLTextInfosetOutputter private (writer: java.io.Writer, pretty:
Boolean, xmlOutputStyle: XMLOutputStyle.Type)
Review Comment:
You should be able to add a default pareamer to the xmlOutputStyle here,
then you don't need the extra constructor. You can still have the null check
inside this class (though, we don't do that for anything else, I'm not sure
it's really necessary).
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/XMLTextInfosetOutputter.scala:
##########
@@ -25,18 +25,23 @@ import org.apache.daffodil.dpath.NodeInfo
import org.apache.daffodil.exceptions.Assert
import org.apache.daffodil.util.Indentable
+import org.apache.daffodil.api.XMLOutputStyle
+
+class NoMatchException(s: String) extends Exception(s) {}
+
/**
* Writes the infoset to a java.io.Writer as XML text.
*
* @param writer The writer to write the XML text to
* @param pretty Whether or to enable pretty printing. Set to true, XML
* elements are indented and newlines are inserted.
Review Comment:
Missing documentation for xmlOutputStyle param.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/XMLTextInfosetOutputter.scala:
##########
@@ -25,18 +25,23 @@ import org.apache.daffodil.dpath.NodeInfo
import org.apache.daffodil.exceptions.Assert
import org.apache.daffodil.util.Indentable
+import org.apache.daffodil.api.XMLOutputStyle
+
+class NoMatchException(s: String) extends Exception(s) {}
Review Comment:
I'm not sure we need the null check, bu if we do, I'd suggest using an
existing exception. I think `Assert.usage` might be similar to what we already
use? E.g.
```scala
Assert.usage(xmlOutputStyle != null, "message here")
```
##########
daffodil-lib/src/main/scala/org/apache/daffodil/api/XMLOutputStyle.scala:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.api
+
+import org.apache.daffodil.util.Enum
+
+object XMLOutputStyle extends Enum {
Review Comment:
For consistency, I think we want specific japi and sapi variants if this
enum. This ensures that the documentation actually ends up in the
Java/Scaladocs (no docs are generated from daffodil-lib) and ensures that all
the "public" API is all in the .japi or .sapi namespace.
For example, we have the ValidationMode enum which is duplicated in the
japi/sapi, and then there is a ValidationConversions object to convert to the
japi/sapi enums to the api enum that is actually used by the underlying
XMLTextInfosetOutputter. It's not the prettiest thing in the world, but it does
ensure that all the public avialble objects are in the .japi and .sapi
namespaces and end up in the java/scala doc.
I also wonder if this enum wants to be in the .infoset namespace instead of
the .api namespace, since it really is only ever going to be used for infoset
clasess?
##########
daffodil-japi/src/main/scala/org/apache/daffodil/japi/infoset/Infoset.scala:
##########
@@ -40,6 +40,8 @@ import org.apache.daffodil.infoset.DIComplex
import org.apache.daffodil.infoset.DIArray
import org.apache.daffodil.dpath.NodeInfo
+import org.apache.daffodil.api.XMLOutputStyle
Review Comment:
I'm not sure if Java users can easily use this enum. See what we do with the
japi for ValidationMode and ValidationConversions as a pattern for how to
handle enums.
--
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]
