stevedlawrence commented on code in PR #1650:
URL: https://github.com/apache/daffodil/pull/1650#discussion_r3266544328
##########
.gitattributes:
##########
@@ -14,4 +14,6 @@
# limitations under the License.
# Do not include KEYS in archived source releases
-/KEYS export-ignore
+/KEYS export-ignore
+# ensure stringAsXml file line endings are not normalized in windows
+/daffodil-test/src/test/resources/org/apache/daffodil/infoset/stringAsXml/**
-text
Review Comment:
Did you look into any other alternatives to this? It would be nice if we
didn't need to have this one special case in a hidden file. It might be hard to
remember that this exists if this ever leads to issues.
If we do keep this, instead of doing a directory glob, can we list the exact
files that this applies to to make it clear exactly which files have this
issue? I think it's only one or two XML files? Is it also possible to add a
comment to those XML files that explains that the file intentionally contains
CR's for testing purposes, and that git's auto.crlf feature is disabled for
this file via the .gitattributes to prevent that from changing?
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -638,40 +648,111 @@ object XMLUtils {
res
}
+ /**
+ * normalizes CRLF to LF within text nodes in non-stringAsXML elements
+ */
+ private def normalizeCRLFtoLF(ns: Node): Node = {
+ if (!ns.isInstanceOf[Elem]) return ns
Review Comment:
Instead of this early return, can we change the default case to `case e:
Elem =>`, and then have the default case be `case _ => ns`? It's maybe a bit
slower since we'll have to do some matching, but it's more scala-y and I
imagine it doesn't make that much of a difference in performance.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -638,40 +648,111 @@ object XMLUtils {
res
}
+ /**
+ * normalizes CRLF to LF within text nodes in non-stringAsXML elements
+ */
+ private def normalizeCRLFtoLF(ns: Node): Node = {
+ if (!ns.isInstanceOf[Elem]) return ns
+
+ ns match {
+ // NOTE: this is specifically for the stringAsXml feature as we avoid
+ // making changes to any of its children requiring that stringAsXml in
+ // the infoset match results exactly.
+ case e @ Elem(
Review Comment:
We have this same kindof-complex match/case in a number of places to check
for the stringAsElem. Thoughts on doing something like this:
```scala
private def isStringAsXmlElem(e: Elem) => e match {
case Elem(null, XMLTextInfoset.stringAsXml, ...) => true
case _ => false
}
```
And then these become something like
```
case e: Elem if isStringAsXmlElem(e) => ...
```
It's also makes it easier to update that one function if we ever change what
it stringAsXml elements look like.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -638,40 +648,111 @@ object XMLUtils {
res
}
+ /**
+ * normalizes CRLF to LF within text nodes in non-stringAsXML elements
+ */
Review Comment:
Can you add something that explain why this is important? Something about
how fields in infosets could contain LF, but could be changed to CRLF due to
git's autocrlf feature. And since infoset outputters always output LF we need
to undo with git might do and normalize those CRLF's to LF.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1055,6 +1155,28 @@ Differences were (path, expected, actual):
computeTextDiff(zPath, tA, tB, maybeType, maybeFloatEpsilon,
maybeDoubleEpsilon)
thisDiff
}
+ case (cA: Comment, cB: Comment) => {
+ val thisDiff = computeTextDiff(
+ zPath,
Review Comment:
It might be worth adding something to zpath to make it clear this is a
comment that is a child of whatever zpath is. Maybe somethin like `zPath +
"/@comment"`. The path step make sit clear it's a child, and `@` is what we've
been using to indicate something is like an attribute or somethig other than an
element. Same idea for pcadata below.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -638,40 +648,111 @@ object XMLUtils {
res
}
+ /**
+ * normalizes CRLF to LF within text nodes in non-stringAsXML elements
+ */
+ private def normalizeCRLFtoLF(ns: Node): Node = {
+ if (!ns.isInstanceOf[Elem]) return ns
+
+ ns match {
+ // NOTE: this is specifically for the stringAsXml feature as we avoid
+ // making changes to any of its children requiring that stringAsXml in
+ // the infoset match results exactly.
+ case e @ Elem(
+ null,
+ XMLTextInfoset.stringAsXml,
+ Null,
+ NamespaceBinding(null, null | "", _),
+ _*
+ ) => {
+ e
+ }
+ case _ => {
+ val e = ns.asInstanceOf[Elem]
+ val children = e.child
+ val normalized = children
+ .map {
+ case Text(data) if data.contains("\r") => {
+ val replaced = data.replaceAll("\r\n", "\n").replaceAll("\r",
"\n")
+ Text(replaced)
+ }
+ case c => c
+ }
+ .map(normalizeCRLFtoLF)
Review Comment:
Might be a bit cleaner to do something like:
```scala
case e: Elem ... => e // stringAsXml case
case e: Elem => {
val normalized = e.child.map(normalizeCRLFtoLF)
val res =
if (normalized eq children) e
else e.copy(child = normalized)
}
}
case Text(data) if data.contains("\r") => {
val replaced = ...
Text(replaced)
}
case _ => n
```
This makes it a bit more clear that it just recurses down
non-stringAsXmlElements and the only thing that actually changes is Text nodes
that contain CR's.
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/xml/XMLUtils.scala:
##########
@@ -1055,6 +1155,28 @@ Differences were (path, expected, actual):
computeTextDiff(zPath, tA, tB, maybeType, maybeFloatEpsilon,
maybeDoubleEpsilon)
thisDiff
}
+ case (cA: Comment, cB: Comment) => {
+ val thisDiff = computeTextDiff(
+ zPath,
+ cA.toString,
+ cB.toString,
+ maybeType,
+ maybeFloatEpsilon,
+ maybeDoubleEpsilon
Review Comment:
Should we pass in None for type, and epsilons for comments and PCData? I'm
not even really sure how maybeType coul dbe defined here. Maybe if stringAsXml
contained something like this:
```xml
<stringAsXml xmlns="">
<foo xsi:type="xs:int">
<!-- inline comment -->
5
</foo>
</stringAsXml>
```
I *think* in that case we might e usuing xs:int for type aware comparisons?
And we'll try to compare the comment as if it were an int, which might break?
I'm not postive, but I think we can avoid this all if we just pass in None for
these to disable type awareness.
--
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]
