fix: html5ever may hide actual errors#704
Conversation
There was a problem hiding this comment.
The standardization logic implemented here seems to be a good balance. It took me a while to understand the intent of the >\n< -> >< replacement (newlines can be added between tags to aid legibility, but spaces around inline tags are not interchangeable with newlines), so maybe that could be clarified somewhere.
Couple comments about the replacements that eat (potentially meaningful) whitespace.
tests/lib.rs
Outdated
| .replace("<br/>", "<br />") | ||
| .replace("<hr>", "<hr />") | ||
| .replace("<hr/>", "<hr />") | ||
| .replace('\t', "") |
There was a problem hiding this comment.
This seems to only affect regression_test_83 where there is
| foo | bar |
→|-----|------|
| baz | alef |
expected to render as
<p>| foo | bar |
→|-----|------|
| baz | alef |</p>
But isn't that incorrect? The leading whitespace should be trimmed in the output.
There was a problem hiding this comment.
Uhm, it seems so, I have to check it. Thanks!
There was a problem hiding this comment.
I think the test was incorrect, now should be right.
|
Yes, this seems like it admits more precise tests. I didn't go through all the changes in detail, so am glad that @ollpu seems to have gone through it in more detail. I'd be quite happy for them to give a final approval if they're willing to spend the time. |
|
Finally! |
Fixes: #702.
html5ever could be removed if this change is approved (pending commit in this pull request).
Please, @ollpu, @raphlinus, review this pull request. This change makes more difficult creating tests but the avoiding of false negatives may pay off.