Skip to content

fix(XMLSerializer): Preserve whitespace character references#310

Merged
karfau merged 12 commits into
masterfrom
284-serialize-whitespace-literals-to-character-references
Sep 14, 2021
Merged

fix(XMLSerializer): Preserve whitespace character references#310
karfau merged 12 commits into
masterfrom
284-serialize-whitespace-literals-to-character-references

Conversation

@karfau

@karfau karfau commented Aug 29, 2021

Copy link
Copy Markdown
Member

BREAKING CHANGE: If you relied on the not spec compliant preservation of \t, \n or \r in attribute values. To preserve those you will have to create XML that instead contains the correct numerical (or hexadecimal) equivalent (	, 
, 
).

@karfau karfau added this to the 0.8.0 milestone Aug 29, 2021
@karfau karfau added the breaking change Some thing that requires a version bump due to breaking changes label Aug 29, 2021
@karfau karfau requested a review from brody2consult August 29, 2021 07:33
@brody2consult

Copy link
Copy Markdown
Member

I have a couple of questions:

I am also wondering if xmltest can eventually be considered a total overlap with the tests we have added and fixed, or if we will just keep it as an extra test in the long term.

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks.

It would be nice to get my questions answered but I would consider them to be non-blocking.

@karfau

karfau commented Aug 29, 2021

Copy link
Copy Markdown
Member Author

I am also wondering if xmltest can eventually be considered a total overlap with the tests we have added and fixed, or if we will just keep it as an extra test in the long term.

My perspective on xmltest is that it provides an orientation regarding going into the "right"/expected direction (actual getting closer to or same as expected) and I would prefer to keep this, even if it's treated as a separate test run (e.g. only after the other tests passed).
The drawback is that they are not very good at communicating why something is expected.

The more I work with all the tests, the clearer the redundancies get, despite all the chaos. I think at some point not to far in the future we will be able to unclutter this without loosing any coverage/mutation score.

Regarding comments in lib, I usually put them to make things more obvious.
The git history always provides more information on details. But I'll check.

@karfau karfau left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Your suggestions for additional comments make total sense

Comment thread lib/dom.js Outdated
Comment thread test/parse/parse-element.test.js
@karfau karfau force-pushed the 284-serialize-whitespace-literals-to-character-references branch from 5217b37 to 6840849 Compare August 29, 2021 19:56

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with a couple more nits. Yeah your comments make total sense.

Comment thread lib/dom.js Outdated
Comment thread lib/dom.js Outdated
@karfau karfau force-pushed the 284-serialize-whitespace-literals-to-character-references branch from ff7210e to 63538b3 Compare September 2, 2021 06:19
@karfau karfau changed the base branch from master to 49-more-linebreaks September 2, 2021 06:19
Base automatically changed from 49-more-linebreaks to master September 9, 2021 06:09
@karfau karfau force-pushed the 284-serialize-whitespace-literals-to-character-references branch 2 times, most recently from f204e98 to 84ef8c7 Compare September 9, 2021 14:12

@brody2consult brody2consult left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still LGTM with a minor comment / question

Comment thread lib/dom-parser.js Outdated
Comment thread lib/dom.js
Comment on lines +1140 to +1141
* > The replacement text of any entity referred to directly or indirectly
* > in an attribute value must not contain a <.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 for starting quoted lines with > here 👍

@karfau karfau merged commit 40745c6 into master Sep 14, 2021
@karfau karfau deleted the 284-serialize-whitespace-literals-to-character-references branch September 14, 2021 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Some thing that requires a version bump due to breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect serialization of XML attributes containing white space

2 participants