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

best practices for encoding control characters? #164

Open
michaelglass opened this issue Apr 14, 2021 · 3 comments
Open

best practices for encoding control characters? #164

michaelglass opened this issue Apr 14, 2021 · 3 comments

Comments

@michaelglass
Copy link

michaelglass commented Apr 14, 2021

When encoding terminal output that includes control characters (e.g. ESC) into XML, renderText produces valid utf-8 that's invalid XML.

Is there some established way of handling this case? If not, If I were to make a PR to make renderText more resilient to this, what would be the preferred direction?

  • escaping the chars somehow
  • filtering them out
  • wrapping them in CDATA (is this valid? I'm not really an XML pro)
  • throwing / returning an either
@k0ral
Copy link
Collaborator

k0ral commented Apr 14, 2021

Although it's not advertised anywhere in its documentation (as far as I checked), xml-conduit follows version 1.0 of XML standard.
As XML 1.0 explicitly forbids most C0 control codes (only TAB, LF and CR are allowed), it looks like we won't get away without bending some laws.

If I had to make a suggestion, I would recommend:

Regarding your other proposals:

  • "filtering them out" tampers with the content of the XML document, such that the result may not be meaningful anymore to the user ; I would advise against it, even as an opt-in feature;
  • "wrapping them in CDATA" is invalid according to XML 1.0 specification, unless I am mistaken;
  • "throwing / returning an either" would be my 2nd choice, as it would make the behavior more correct, albeit less useful to (some) users.

@michaelglass
Copy link
Author

should I close this issue or wait until I open a PR?

@k0ral
Copy link
Collaborator

k0ral commented Apr 15, 2021

I suggest we keep this issue open until a fix is merged.
FYI, you can link a PR to an issue, such that merging the former automatically closes the latter.

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

No branches or pull requests

2 participants