A code fence keeps the blank lines in its payload - #280
Merged
Conversation
`trim($content, "\n")` removed every leading and trailing newline from a fenced
code block's payload, which is lossy in both directions and disagrees with
djot.js:
``` djot.js: <pre><code>\n</code></pre>
(blank) was: <pre><code></code></pre>
```
A fence holding one blank line and a fence holding no lines at all both became
"", so two different documents produced the same AST. A leading blank line
disappeared the same way: "```\n\nx\n```" rendered as if the blank were not
there.
The payload is stored verbatim now, its own line terminator included. The HTML
renderer needed no change - it appends a newline only when the content does not
already end in one - and the official djot suite is unchanged at 284/284.
THE MARKDOWN RENDERER DID need one. It appended "\n" unconditionally before the
closing fence, which doubled the terminator once the payload carried its own and
grew a blank line on every round trip. It now supplies one only when the payload
does not.
HOW IT WAS FOUND. 0.1.33's borrowed fast path renders these correctly, so the
two paths disagreed and the output depended on which one ran - a document with a
blank-payload fence rendered one way alone and another way beside a loose list,
because the loose list sends the whole document to the authoritative pipeline.
A differential sweep crossing every construct the fast path claims against every
other at three separator widths reported 124 divergences over 411 rendered
pairs, every one of them this defect. The same sweep reports 0 after this
change.
Three tests asserted the trimmed shape and now assert the reference shape; the
one named for trimming is renamed, since it pinned the divergence rather than a
requirement. One test is added for the case the trim made unrepresentable - that
an all-blank fence is not an empty one.
Note for a follow-up rather than this change: `tryParseRawBlock` carries the same
`trim($content, "\n")` at the same line position in its own function. It is the
same pattern, but no divergence was measured for it here, so it is left alone.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #280 +/- ##
============================================
- Coverage 92.19% 92.17% -0.02%
- Complexity 3967 3969 +2
============================================
Files 111 111
Lines 11066 11067 +1
============================================
- Hits 10202 10201 -1
- Misses 864 866 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A fenced code block loses the blank lines in its payload, and the 0.1.33 fast
path is what made it visible.
The defect
BlockParserstored the payload astrim($content, "\n"), which removes everyleading and trailing newline. That is lossy in both directions:
<pre><code>\n</code></pre><pre><code></code></pre><pre><code>\n\n</code></pre><pre><code></code></pre>x<pre><code>\nx</code></pre><pre><code>x</code></pre><pre><code></code></pre>A fence holding one blank line and a fence holding none both became
"", so twodifferent documents produced the same AST and there was no way to tell them
apart downstream.
Why now: the two paths disagree
0.1.33's borrowed fast path renders all of these correctly. So the output
depends on which path runs, and the release note's "byte-identical to the
owned-AST result" does not hold for them:
renders
<pre><code>\n</code></pre>on its own - the fast path takes it - and<pre><code></code></pre>when a loose list appears anywhere in the samedocument, because the loose list sends the whole document to the authoritative
pipeline.
A differential sweep crossing every construct the fast path claims against every
other, at three separator widths, reported 124 divergences over 411 rendered
pairs (357 pairs are handed back and never render on the fast path). Every one
of the 124 was this defect. After this change the same sweep reports 0.
The change
BlockParserstores the payload verbatim, its own terminator included.when the content does not end in one.
"\n"unconditionally before theclosing fence, which doubled the terminator once the payload carried its own
and grew a blank line on every round trip. It now supplies one only when the
payload does not. This was caught by the existing round-trip test, not by me.
Tests
reference shape.
testCodeBlockTrimsLeadingAndTrailingBlankLinesis renamed -it pinned the divergence rather than a requirement, and its input renders
<pre><code>\nbin/cake linter\n\n</code></pre>in djot.js.is not an empty one.
cs-checkandstanclean.Not in this change
tryParseRawBlockcarries the sametrim($content, "\n")in its own function.It is the same pattern and probably the same defect, but I measured no
divergence for it here, so it is left for a separate look rather than changed on
a guess.
Found while checking whether the 0.1.33 fast path carried defects already fixed
in the Carve engines, which are downstream of this parser. It does not - the
regressions went the other way, and this one is fixed there already.