Skip to content

fix(asciidoc): keep /// as text and support longer comment delimiters - #4394

Open
r0h1tb wants to merge 1 commit into
docling-project:mainfrom
r0h1tb:fix/asciidoc-listing-blocks
Open

r0h1tb wants to merge 1 commit into
docling-project:mainfrom
r0h1tb:fix/asciidoc-listing-blocks

Conversation

@r0h1tb

@r0h1tb r0h1tb commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Main skips // lines and //// blocks (#4431), but:

  • /// is treated as a line comment and dropped, although AsciiDoc only treats exactly two slashes as one.
  • Comment delimiters longer than four slashes (/////, ...) aren't recognised, so the block's content leaks into the output.
  • An unclosed //// hides the rest of the document.

Fix

On top of the existing _LINE_COMMENT_RE / comment-delimiter logic:

  • _LINE_COMMENT_RE matches exactly two slashes.
  • Any line of four or more slashes opens a comment block that only an identical line closes. Closers are indexed once, so many unmatched markers don't rescan the document.
  • A delimiter with no matching closer breaks the paragraph and the rest of the document is kept, like the stray -- handling from fix(asciidoc): turn listing blocks into code items #4400.

Comments nested inside ==== / **** / ____ / -- blocks are left for #4593.

Tests

New cases for /// text, longer delimiters needing an identical closer, unclosed delimiters of each length, many unmatched delimiters without rescanning, and comment markers inside ---- / .... staying code. 7 of them fail on main; the two literal-block controls pass both ways. tests/test_backend_asciidoc.py: 51 passed. Ruff format/check clean, ty reports the same 3 existing warnings as main, and the max-lines hook passes at its 1500 limit.

Checklist:

  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

@github-actions

Copy link
Copy Markdown
Contributor

✅ DCO Check Passed

Thanks @r0h1tb, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

PeterStaar-IBM
PeterStaar-IBM previously approved these changes Sep 28, 2026

@PeterStaar-IBM PeterStaar-IBM 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!

@PeterStaar-IBM

Copy link
Copy Markdown
Member

@r0h1tb Can you fix the merge conflict? All the rest looks good to merge

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dolfim-ibm

Copy link
Copy Markdown
Member

@r0h1tb is this a duplicate of #4400?

@r0h1tb
r0h1tb force-pushed the fix/asciidoc-listing-blocks branch from e648805 to 2ae54ce Compare October 5, 2026 14:51
@r0h1tb r0h1tb changed the title fix(asciidoc): read listing blocks as code and skip comments fix(asciidoc): skip comments instead of emitting them as text Oct 5, 2026
@r0h1tb

r0h1tb commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@dolfim-ibm partly, yes. #4400 landed the listing-block half first, so I rebased onto main and dropped mine. What's left is the comment handling (// lines and //// blocks), which main still emits as text. Updated the title and description to match, and the conflict is gone.

ceberam
ceberam previously approved these changes Oct 5, 2026

@ceberam ceberam 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.

Thanks @r0h1tb for the contribution!

I agree that leaking // TODO ask legal and similar comment noise into converted output is a real bug, and the current behavior of silently dropping comment lines and blocks is the right default for most users.

However, silently dropping the content is an irreversible loss. AsciiDoc comments may also be used for editorial annotations, review notes, conditional-content explanations, and disabled-but-meaningful blocks, exactly the kind of content that is valuable for downstream AI workflows such as RAG pipelines and document summarization. Even for the use case that you described (the TODO ask legal notes), the comments may be useful for agents that would run tools to pull external (legal) information.

The Word, Excel, and PowerPoint backends all solve this with ContentLayer.NOTES. Comments are emitted into that layer, which is excluded from Markdown and HTML export by default. A user who wants the comment content can pass the NOTES layer to the serializer explicitly.

This is a zero-regression approach: the default output is identical to what this PR produces, but the content is not destroyed.

Suggested follow-up implementation

Add a small sentinel dataclass alongside the existing _LiteralBlock:

@dataclass(frozen=True)
class _CommentBlock:
    text: str  # raw comment text, delimiters stripped

Yield it from _iter_blocks instead of advancing past the comment lines silently. Then handle it in _parse:

elif isinstance(block, _CommentBlock):
    doc.add_text(
        text=block.text,
        label=DocItemLabel.PARAGRAPH,
        content_layer=ContentLayer.NOTES,
    )

My suggestion is:

  1. Merge this PR as-is
  2. Open a follow-up issue to re-capture AsciiDoc comments into ContentLayer.NOTES, mirroring the Office backend pattern.
  3. Optionally, add a capture_comments: bool = True flag to AsciiDocBackendOptions in that follow-up, so users can opt out of capturing comments entirely if they prefer.

@r0h1tb

r0h1tb commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Makes sense, the NOTES layer is a better home for them than dropping them. Opened #4593 for it, and I'm happy to pick it up once this one is in.

The red check is the ML job hitting its 60-minute limit while stuck on apt-get update, before any tests ran, so a re-run should clear it. I don't have the rights to re-run it myself.

@ceberam

ceberam commented Oct 5, 2026

Copy link
Copy Markdown
Member

Makes sense, the NOTES layer is a better home for them than dropping them. Opened #4593 for it, and I'm happy to pick it up once this one is in.

The red check is the ML job hitting its 60-minute limit while stuck on apt-get update, before any tests ran, so a re-run should clear it. I don't have the rights to re-run it myself.

Thanks @r0h1tb for taking this step forward, but let's check first if the other reviewers agree with the approach. FYI @PeterStaar-IBM @dolfim-ibm

@dolfim-ibm

Copy link
Copy Markdown
Member

@r0h1tb this needs another rebase: #4431 landed comment skipping on main (// lines and //// blocks, with tests), so most of this change is already in. What's left worth keeping is narrower:

Could you rebase onto main, keep only those deltas on top of the existing _LINE_COMMENT_RE / _COMMENT_BLOCK_DELIMITER logic, and turn the test into cases for them? Comments nested inside ==== / **** / ____ / -- blocks still leak as text on main too; handling that here would be welcome, otherwise it can go with #4593.

On #4593: agreed with @ceberam, NOTES layer is the right home.

…closed blocks

Main already skips // lines and //// blocks. On top of that:

- /// is not a line comment and stays as text.
- Comment delimiters longer than four slashes open a block that only an
  identical line closes.
- A comment delimiter with no matching closer breaks the paragraph
  instead of hiding the rest of the document, like a stray "--".

Signed-off-by: Rohit Behera <126186063+r0h1tb@users.noreply.github.com>
@r0h1tb
r0h1tb force-pushed the fix/asciidoc-listing-blocks branch from 2ae54ce to bf9214b Compare October 19, 2026 06:47
@r0h1tb r0h1tb changed the title fix(asciidoc): skip comments instead of emitting them as text fix(asciidoc): keep /// as text and support longer comment delimiters Oct 19, 2026
@r0h1tb

r0h1tb commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and narrowed to the three deltas on top of the existing comment logic: /// stays text, delimiters longer than four slashes need an identical closer, and an unclosed delimiter no longer hides the rest of the document. The tests are now cases for those. I left comments nested inside ==== / **** / ____ / -- blocks for #4593.

@github-actions github-actions Bot added ai:ci-safe AI triage found no CI-safety concern (advisory) asciidoc issue related to asciidoc backend bug Something isn't working labels Oct 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

AI triage (advisory)

Checked commit bf9214bf7c. The analysis read the diff only. It did not run code from this pull request.

The PR changes the AsciiDoc backend so that /// lines stay as body text and only exactly two slashes start a line comment. It also accepts comment block delimiters of four or more slashes, which need an identical closer, and keeps the rest of the document when a delimiter has no closer.

Topic labels added: bug, asciidoc.

CI safety: no concern found. A maintainer must still approve CI.

3 related pull request(s) in the same code
PR State Signal Assessment
#4663 open also references issue #4593 related: It edits the same comment logic in _iter_blocks, but it adds a feature that keeps comments in the notes content layer. It does not add the exact-two-slash rule or the longer delimiters. It also treats an unclosed //// as a comment to the end of the document, which conflicts with this PR, so the two PRs will conflict when merged.
#4431 merged referenced as #4431 related: It is merged and added the base // and //// comment skipping that this PR refines. This PR does not repeat that change.
#4400 merged referenced as #4400 related: It is merged and added listing block and content block delimiter handling, including the stray -- rule that this PR follows. This PR does not repeat that change.

The ai:* labels are advisory. They do not approve CI and they do not add tests:full. A new push runs this check again.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai:ci-safe AI triage found no CI-safety concern (advisory) asciidoc issue related to asciidoc backend bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants