Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape end of line characters based on 'XML End-Of-Line Encoding' SEP. #349

Merged
merged 2 commits into from
May 14, 2021

Conversation

kggilmer
Copy link
Contributor

@kggilmer kggilmer commented May 14, 2021

Issue #, if available: #142

SEP: /reviews/CR-40585029/revisions/3

NOTE: This PR does not address linefeeds in attribute string values. Discussed this w/ SEP author and believe this needs to be implemented to conform to SEP. However the XPP API does not seem to offer a way of writing raw character values to attributes. So, in order to make this work I believe we'll have to move to another serialize that provides us more control.

Description of changes:

  • Escape text elements for specific line feed characters per SEP linked above

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kggilmer kggilmer requested review from aajtodd and ianbotsf May 14, 2021 01:47
Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* | `YQ3ChSBiwoU=` | `'a\r\u0085 b\u0085'` | `'a
… b…'` |
* | `YQ3igKggYsKFIGPigKg=` | `'a\r\u2028 b\u0085 c\u2028'` | `'a

 b… c
'` |
*/
// FIXME ~ Unable to add "org.junit.jupiter:junit-jupiter-params" as a test dependency...classes do not resolve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

or you could just write your own quick loop, something like:

// list of (raw input, expected) pairs
val tests = listOf(
    "\n \n" to "
 
",
    ...
)

tests.forEachIndexed { idx, test -> {
    val writer = xmlStreamWriter(pretty = false)
    writer.startTag("a")
    writer.text(test.first)
    writer.endTag("a")

    val expected = "<a>${test.second}</a>"
    assertEquals(expected, writer.toString(), "test case $idx failed")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's better I'll update before push.

@@ -59,7 +59,15 @@ class XmlPullSerializer(pretty: Boolean, private val serializer: XmlSerializer =
}

override fun text(text: String): XmlStreamWriter {
serializer.text(text)
text.forEach { character ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question

  1. It looks as though serializer.text(...) works as "append" if called multiple times? (own clarification)
  2. Is there any performance consideration here?

Copy link
Contributor Author

@kggilmer kggilmer May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes this is my understanding as well. I would expect unit tests to fail if this were not the case after the addition. Here is the library function that appends to the string.
  2. Yes. Optimistically perhaps not much of an impact as we're simply pulling the linear iteration on each character in the string from inside the xpp library into our code, but there may be things I'm missing. In any case I was unable to think of a cleaner or more performant way of implementing this due to the restrictions in the XPP interface itself. I think the way forward here is to move from XPP to our own implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure sounds good. I sort of figured as much

@kggilmer kggilmer merged commit 86756cc into main May 14, 2021
@kggilmer kggilmer deleted the feat-xml-eol-escaping branch May 14, 2021 18:02
@aajtodd aajtodd mentioned this pull request May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants