You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mbeckerle (via GitHub)" <gi...@apache.org> on 2024/05/30 16:43:56 UTC
[PR] Added test to show preexisting PUA chars are preserved. [daffodil]
mbeckerle opened a new pull request, #1250:
URL: https://github.com/apache/daffodil/pull/1250
See test_puaPreexistingInfosetChars
test_puaPreexistingInfosetChars_remapped
Note that a JSON workaround was required in the TDML runner.
DAFFODIL-2883
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added test to show preexisting PUA chars are preserved. [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1250:
URL: https://github.com/apache/daffodil/pull/1250#discussion_r1621080114
##########
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/TDMLInfosetInputter.scala:
##########
@@ -81,7 +82,12 @@ class TDMLInfosetInputter(
case _ if (firstVersion eq null) => ""
// the json infoset inputter maintains CRLF/CR, but XML converts CRLF/CR to
// LF. So if this is Json, then we want the CRLF/CR converted to LF
- case jsonii: JsonInfosetInputter => firstVersion.replaceAll("(\r\n|\r)", "\n")
+ // Furthermore, the json infoset inputter maintains Unicode PUA characters, the XML
+ // infoset inputters have to remap some characters from the PUA back to the
+ // XML-illegal characters. For sake of comparison with other XML infoset inputters
+ // we also remap AS IF it were XML.
+ case jsonii: JsonInfosetInputter =>
+ XMLUtils.remapPUAToXMLIllegalCharacters(firstVersion.replaceAll("(\r\n|\r)", "\n"))
Review Comment:
Can we remove the replaceAll? I think remapPUAToXMLIllegalCharacters also remaps CRLF/CR to LF?
Which makes sense. XML infoset outputters remap characters so we need to do the same for JSON for TDML tests to verify it's all the same.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added test to show preexisting PUA chars are preserved. [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle merged PR #1250:
URL: https://github.com/apache/daffodil/pull/1250
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added test to show preexisting PUA chars are preserved. [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1250:
URL: https://github.com/apache/daffodil/pull/1250#discussion_r1622702667
##########
daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testUnparserGeneral.tdml:
##########
@@ -104,30 +112,82 @@
</tdml:parserTestCase>
-<!--
- Test Name: puaInfosetChars_03
+ <!--
+ Test Name: puaInfosetChars_CR_CRLF_01
Schema: illegalChars
- Purpose: When parsing illegal XML characters, they get mapped to PUA. This test demonstrates that unparsing and unparsing actual PUA values is handled correctly.
+ Purpose: When unparsing XML that has the remapped CR (0xE00D) code point, those get remapped back to actual CR.
+ Note that unparsing (without raund trip) never deals with CR/CRLF replacement by LF in XML.
Review Comment:
typo: round
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added test to show preexisting PUA chars are preserved. [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1250:
URL: https://github.com/apache/daffodil/pull/1250#discussion_r1622514929
##########
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/TDMLInfosetInputter.scala:
##########
@@ -81,7 +82,12 @@ class TDMLInfosetInputter(
case _ if (firstVersion eq null) => ""
// the json infoset inputter maintains CRLF/CR, but XML converts CRLF/CR to
// LF. So if this is Json, then we want the CRLF/CR converted to LF
- case jsonii: JsonInfosetInputter => firstVersion.replaceAll("(\r\n|\r)", "\n")
+ // Furthermore, the json infoset inputter maintains Unicode PUA characters, the XML
+ // infoset inputters have to remap some characters from the PUA back to the
+ // XML-illegal characters. For sake of comparison with other XML infoset inputters
+ // we also remap AS IF it were XML.
+ case jsonii: JsonInfosetInputter =>
+ XMLUtils.remapPUAToXMLIllegalCharacters(firstVersion.replaceAll("(\r\n|\r)", "\n"))
Review Comment:
> Can we remove the replaceAll? I think remapPUAToXMLIllegalCharacters also remaps CRLF/CR to LF?
>
> Which makes sense. XML infoset outputters remap characters so we need to do the same for JSON for TDML tests to verify it's all the same.
No this is required.
Remapping XML illegal characters to PUA also maps CRLF and CR to LF,
But this is the opposite direction. from PUA back to "native" string. We need the additional CR/CRLF => LF because it didn't happen during the parse into a JSON infoset.
I'm adding clarification to the code comments about this.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org