stevedlawrence commented on code in PR #1298:
URL: https://github.com/apache/daffodil/pull/1298#discussion_r1778483820
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JsonInfosetInputter.scala:
##########
@@ -118,10 +123,11 @@ class JsonInfosetInputter(input: java.io.InputStream)
extends InfosetInputter {
primType: NodeInfo.Kind,
runtimeProperties: java.util.Map[String, String]
): String = {
- if (jsp.getCurrentToken() == JsonToken.VALUE_NULL) {
+ if (!jsp.getCurrentToken().isScalarValue()) {
+ throw new
NonTextFoundInSimpleContentException(jsp.getCurrentToken().asString())
Review Comment:
I think we can make this diagnostic more helpful to users. Maybe something
like this:
```scala
"Unexpected array or object '" + getLocalName + "' on line " +
jsp.getTokenLocation().getLineNr()
```
We should have a test that this fails correctly for both arrays and objects.
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JsonInfosetOutputter.scala:
##########
@@ -112,9 +112,24 @@ class JsonInfosetOutputter private (writer:
java.io.BufferedWriter, pretty: Bool
} else {
simple.getText
}
- writer.write('"')
- writer.write(text)
- writer.write('"')
+ if (simple.metadata.dfdlType == DFDLPrimType.String ||
+ simple.metadata.dfdlType == DFDLPrimType.HexBinary ||
+ simple.metadata.dfdlType == DFDLPrimType.AnyURI ||
+ simple.metadata.dfdlType == DFDLPrimType.DateTime ||
+ simple.metadata.dfdlType == DFDLPrimType.Date ||
+ simple.metadata.dfdlType == DFDLPrimType.Time ||
+ (simple.metadata.dfdlType == DFDLPrimType.Double &&
text.equals("NaN")) ||
+ (simple.metadata.dfdlType == DFDLPrimType.Double &&
text.equals("INF")) ||
+ (simple.metadata.dfdlType == DFDLPrimType.Double &&
text.equals("-INF")) ||
+ (simple.metadata.dfdlType == DFDLPrimType.Float &&
text.equals("NaN")) ||
+ (simple.metadata.dfdlType == DFDLPrimType.Float &&
text.equals("INF")) ||
+ (simple.metadata.dfdlType == DFDLPrimType.Float &&
text.equals("-INF"))) {
Review Comment:
It's usually best to avoid string comparisons when there's something more
specific available. String comparisons can silently break if we ever decide to
change the string representation but don't check the condition. For example,
`Double` and `Float` have `isInfinite` and `isNaN` functions.
This is also a pretty big if-statement. I might suggest moving this all to a
separate private function, something like this:
```scala
private def needsQuote(simple: InofsetSimpleElement): Boolean = {
simple.metadata.dfdlType match {
case DFDLPrimType.String => true
case DFDLPrimType.HexBinary => true
case DFDLprimType.AnyURI => true
...
// json does not support inf/nan double/float so they must be quoted
case DFDLPrimType.Double => {
val d = simple.getDouble
d.isInfinite || d.isNaN
}
case DFDLPrimType.Float => {
val f = simple.getFloat
f.isInfinity || f.isNan
}
case _ => false
}
```
Then this is statement just becomes `if (needsQuotes(simple)) ....`. This
makes it much easier to understand what this if-statement is checking for
without having to decipher it. And it makes it easier to add comments for
unexpected edgecases like inf/nan support.
--
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]
