Repository navigation
fix(md): keep inline code spans inside GFM table cells - #4327
Conversation
|
✅ DCO Check Passed Thanks @kevin9327, all your commits are properly signed off. 🎉 |
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 Require two reviewer for test updatesWhen test data is updated, we require two reviewers
|
|
Hello, It's been a very bad start from me I panicked I wanted to fix my problem and thought that AI will do it all by itself without reading properly the documents you sent me. I learned my mistake and hope that I will learn fast and contribute to your community. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ceberam
left a comment
There was a problem hiding this comment.
Thanks @kevin9327 for the well-described bug report and fix.
The direction of the fix (detecting self.in_table and keeping the span in the buffer) is right. There are, however, several correctness issues that must be resolved before merging, plus a deeper architectural point that changes the scope of the required solution (the issue 1 below). Please, let us know if you would like to address it in this PR, otherwise you could skip the point 1 and we will later handle it in a separate PR.
Blocking issues
1. The fix loses information that belongs in the document model
Beyond being a bug fix, this PR should be a fidelity improvement. Outside of a table, the backend emits a CodeItem for a CodeSpan, so a downstream HTML exporter can render `build` as <code>build</code>. Inside a table cell the current approach — concatenating the code span's raw text into the string buffer — throws that semantic away permanently: a TableCell with text="run build now" is indistinguishable from one that was always plain text.
The correct model already exists in the codebase: RichTableCell (used by the HTML, DOCX, and OpenDocument backends). It carries a ref pointing to a GroupItem whose children are the per-span TextItem / CodeItem nodes, so the full document structure is preserved. The fix should emit RichTableCell for any cell that contains at least one CodeSpan (and, while at it, for cells with bold, italic, or link spans — the same data loss applies to those today).
This requires replacing the md_table_buffer: list[str] per-row string accumulator with a structured per-cell accumulator that tracks spans with type information. When _close_table fires:
- Cells whose span list is entirely plain text →
TableCell(no change for the common case, no performance impact). - Cells with at least one code or formatted span → create a
GroupItemparented to theTableItem, add each span as a childCodeItemorTextItem, then construct aRichTableCell(ref=group.get_ref(), text=plain_text, ...).
The _group_cell_elements helper in msword_backend.py and process_rich_table_cells in html_backend.py are good references for the grouping pattern.
2. Adjacent whitespace is silently dropped (regression introduced by this PR)
The RawText handler calls original_text.strip() before appending to the table buffer. For a cell like run \build` now`, Marko emits three inline nodes:
RawText "| run "
CodeSpan "build"
RawText " now | builds it |"
After stripping, the RawText segments become "| run" and "now | builds it |". The code span text is then concatenated without any separator, yielding "runbuildnow" instead of "run build now".
The test was written to accommodate this regression: lines 263–267 use in membership checks rather than an equality assertion, and the comment at line 263 explicitly says "spaces around the span may collapse". A regression test that knowingly accepts broken output is not a regression test.
Fixing this in the context of issue 1 above is straightforward: the structured cell accumulator records each RawText segment as-is (unstripped), and stripping happens only at final cell extraction — _split_table_row already calls cell.strip() on each cell boundary.
3. A pipe inside a code span breaks cell boundaries
A code span containing a literal | — e.g., `a | b` — should be part of the cell text, not a column separator. The current implementation appends the raw unescaped text (including the |) to the buffer; _split_table_row then splits on it, breaking the cell in two. The RawText table path avoids this problem by using _unescape_except_pipe so that HTML-entity-encoded pipes survive the split. A literal | arriving from a CodeSpan needs the same protection: before buffering, encode it as | so _split_table_row does not treat it as a delimiter. This is handled naturally if the structured-accumulator approach from issue 1 is adopted, because cell boundaries are tracked structurally rather than by scanning for | in a flat string.
4. Inconsistent escaping and double-unescape
The RawText table path uses _unescape_except_pipe at write time, and _close_table applies a final unescape(cellval.strip()) on each cell. The new CodeSpan path calls plain unescape() at write time and the same unescape() fires again in _close_table, double-decoding the content. The HTML unescape function is idempotent for typical entities, but not for all sequences (e.g., &amp; → & → &). The two write paths should use the same escaping strategy, and the buffer contract (whether values are pre-escaped or raw) should be consistent and documented.
Non-blocking issues
5. Add the test case to the shared ground-truth fixture
The inline table with a code-span cell is a good example to include in tests/data/md/sources/inline_and_formatting.md, which already contains a table with bold and italic headers. Adding it there gives future contributors a visible input file and a machine-checked ground truth (both .md and .yaml) that verifies round-trip fidelity. The file is covered by test_convert_valid, which is in the yaml_filter set, so both the Markdown re-export and the full DoclingDocument structure are verified. The standalone test_convert_table_keeps_inline_code_spans test can then be kept as a focused regression guard or removed, depending on preference.
Important: regenerate the ground-truth files (DOCLING_GEN_TEST_DATA=1 uv run pytest) only after the fix is correct, so they reflect the proper output — including RichTableCell content — not the whitespace-collapsed intermediate state.
6. Inline comment narrates history rather than invariant
# Keep the span in the current row. Closing the table here used
# to split one GFM table into two and drop the text before the span.The second and third lines describe what the old code did, which is not useful for a future maintainer deciding whether to modify this branch. Prefer something that describes the rule:
# A CodeSpan does not delimit cells; keep its content in the current buffer slot.7. The _log.debug line is missing from the in_table branch
Every other inline element handler emits a debug log entry. The _log.debug(" - Code Span: %s", element.children) line should be moved to before the if self.in_table branch so it fires in both paths.
|
Ran the current head (60b4ac7) against the table inputs collected in #3991. The code span now stays in the table (one TableItem), though the cell reads The same close-the-table path still fires on other inline nodes, so these still split:
With If it helps the structured-cell work from point 1: marko's GFM table elements ( |
|
Sorry @djlongy , I did not understand how you would like to proceed. There are two options:
In my opinion, it would be easier to proceed with the first option to fix the broken table as soon as possible, and then tackle the refactoring to enhance the parsing of table cells in a separate PR. |
|
@ceberam Thanks for the thorough review. Going with the first option, as you suggest: this PR fixes the broken table (points 2 to 7); the
@djlongy Thanks for running the corpus. The |
4590c89 to
5884d26
Compare
|
Rebased onto The conflict was in the RawText branch. #4313's backslash-escape handling is kept: a |
ceberam
left a comment
There was a problem hiding this comment.
Great, thanks for your contribution @kevin9327 🚀
ceberam
left a comment
There was a problem hiding this comment.
@kevin9327 We had a rather long pipeline of PRs and we have merged them today. Unfortunately this has triggered some conflicts again on your PR. Could you please rebase it to main?
A CodeSpan in a table cell closed the table mid-row, so one GFM table became two and the text before the span was dropped. Signed-off-by: kevin9327 <5299031+kevin9327@users.noreply.github.com>
Review: a RawText segment was stripped before it joined the row, so "run `build` now" became "runbuildnow"; a pipe inside a code span split the cell; and the span's text was decoded once on the way in and again in _close_table. The table buffer now has one contract: _close_table decodes it exactly once, a RawText keeps its whitespace and its pipe entities, and literal text such as a code span is entity-encoded with its pipes as &docling-project#124;. The debug line fires in both paths, the comment states the rule, the fixture gains a code-span row, and the test asserts every cell, including a pipe and an entity inside a span. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: kevin9327 <5299031+kevin9327@users.noreply.github.com>
5884d26 to
d18232f
Compare
|
Rebased onto main (d18232f). The only overlap was #4318, which also appends table-cell text in |
A GFM table cell that contains an inline code span (
run \build` now`) used to close the table at the span. That split one table into two and dropped the text before the backticks.Code spans now stay in the current table row, like emphasis and links already do, and the cell keeps its words around the span.
The table buffer has one contract now, stated next to
_unescape_except_pipe:_close_tabledecodes each cell exactly once, aRawTextkeeps its whitespace and its pipe entities on the way in (so an escaped pipe survives the split), and literal text such as a code span is entity-encoded on the way in with its pipes as|. So\a | b`stays one cell and`&`stays the literal&`.Preserving the span as a
CodeIteminside the cell (RichTableCell, review point 1) is left for a separate change, as suggested in review; this one fixes the broken table.Checklist:
AI-assisted (Grok)