Repository navigation
Conversation
Signed-off-by: Aryan Rajesh Ganjoo <51299383+AryanRG@users.noreply.github.com>
|
✅ DCO Check Passed Thanks @AryanRG, all your commits are properly signed off. 🎉 |
Merge Protections🔴 1 of 2 protections blocking · waiting on 👀 reviews
🔴 Require two reviewer for test updatesWaiting for
This rule is failing.When test data is updated, we require two reviewers
Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Aryan Rajesh Ganjoo <51299383+AryanRG@users.noreply.github.com>
AI triage (advisory)Checked commit The PR makes the JATS backend keep direct Topic labels added: CI safety: no concern found. A maintainer must still approve CI. 1 related pull request(s) in the same code
Reference data changes
Assessment: expected. The The |
There was a problem hiding this comment.
AI first review (advisory): no blocker found
This review covers the full diff at ad7506810f. It read the code and did not run it.
The change keeps table notes visible by using an empty FOOTNOTE wrapper with the note text in child runs. This is a workaround for the table serializer in docling-core 2.100.0. I could not read the installed docling_core source, so I did not check the claim about how its serializers treat referenced footnotes.
A maintainer makes the merge decision. This review never approves the pull request.
| ] | ||
| # Released core table serializers skip referenced footnote wrappers. | ||
| # Keep all content in child runs so it is also exported by traversal. | ||
| footnote = doc.add_text( |
There was a problem hiding this comment.
question: Empty-text FOOTNOTE wrapper depends on one docling-core serializer behavior
Each note is a FOOTNOTE item with text="". The real text sits in child runs, and table_item.footnotes points to the empty wrapper. The code comment says this only works because released core skips referenced wrappers but still walks their children. pyproject.toml allows docling-core>=2.98.0,<3.0.0. The PR evidence covers only 2.100.0.
Please show two things:
- The oldest allowed core (2.98.0) still exports these notes in Markdown and HTML.
- A core release that prints
table.footnotestext by reference does not add an empty line or duplicate the note. In that case the child runs would print a second time.
If one of these fails, either raise the core lower bound or add a test that fails on the problem. If you cannot show both, say that this layout is a deliberate workaround.
There was a problem hiding this comment.
Thanks, I checked both points against ad75068.
-
Oldest allowed core (
2.98.0). Withdocling-core[chunking]==2.98.0,tests/test_backend_jats_table_footnotes.pypasses: 22 passed. The chunking extra is needed for theHierarchicalChunkerimport and matches the project's test dependency. I also exported 12 synthetic JATS cases containing 15 table notes—plain text, literal xrefs, labels, styled/linked/formula runs, multiple paragraphs, and multiple tables—to Markdown and HTML, before and after a JSON round trip. Every expected note fragment appears exactly once, with no empty footnote div or whitespace-only line. -
A core that prints
table.footnotesby reference. The released table serializers I checked (2.98.0,2.100.0, and2.101.1) and currentdocling-coremain do not callserialize_footnotes. The closest candidate is docling-core#675 (open at11bb2a7, based on core2.86.0, below Docling's current2.98.0floor, so the current converter cannot import against it). Running the JATS backend directly against that head, note text is not duplicated: its hook reads only the wrapper's emptytext, while traversal emits the child runs once. It does add empty output, though: one<div class="footnote"></div>per note in HTML, and a whitespace-only line in Markdown when a table has two or more notes.
So the oldest supported release works, but the future-serializer case is only partly clean. The empty-text wrapper is a deliberate workaround for current released serializers, which skip referenced footnote items while still walking their children. Raising the lower bound would not help because no checked release has the by-reference behavior. If that behavior lands in core, this layout should be revisited. I can add a guard test here that fails on an empty footnote div or whitespace-only line if that would be useful.
JATS conversion drops direct
table-wrap-foot/fnnotes. This change attaches each nonempty note as oneFOOTNOTEowned and referenced by its table, preserving source order, formatting, hyperlinks, inline formulas and literal citations such as[8]. Adjacent paragraphs retain a word boundary, including when the next paragraph already starts with whitespace.Refs #4796. This covers table notes only; author notes, nested
fn-groupcontainers, baretable-wrap-footparagraphs and xref resolution remain separate work. Existingfn-groupbehavior is unchanged.Released-core compatibility
Table notes use an empty
FOOTNOTEwrapper with inline child runs. This keeps the note visible with releaseddocling-core2.100.0, whose table serializer skips referenced footnote wrappers but still emits their traversed children. Text child runs retain theFOOTNOTElabel so label filtering and chunk metadata classify them as note content; formula runs retainFORMULA.The change does not modify
pyproject.tomloruv.lock.Checklist:
Review evidence:
docling-core2.100.0:tests/test_backend_jats.pyplustests/test_backend_jats_table_footnotes.py— 108 passed.make validate— passed, including Ruff, formatter, ty, Tach, module coverage and max-lines checks.git diff --check— clean.Exactly six existing references change: JSON, Markdown and indented text for
pntd.0008301.nxmlandpone.0234687.nxml. They represent 20 notes across six tables. TheponeJSON and indented-text semantic changes are child-run label changes fromTEXTtoFOOTNOTE;pntdalso gains the empty-wrapper/inline-group structure needed for released-core export. Markdown changes place each source note in its own paragraph after the table and before following content. No table data, note text, formatting, links, formulas, ownership or source order changes.Known compatibility limit: with released core, excluding only the
FOOTNOTElabel removes note text runs but can leave an inline formula because formulas intentionally retain theFORMULAlabel. A core serializer with subtree-aware table-footnote handling can filter the complete note. Default Markdown/HTML export, ordering and duplication behavior are covered here.Reference-data changes require two approving reviewers under the repository contribution policy.