Repository navigation
fix(html): remove broken obsolete heading hook - #2618
Thota Shashank (thotashashank302) wants to merge 3 commits into
Conversation
|
@microsoft-github-policy-service agree |
XU (kokokoXUY)
left a comment
There was a problem hiding this comment.
Verdict: the removal is correct for markdownify >= 1.0 and I would merge it. Two things are worth addressing first: the new tests also pass on the base commit, so they do not cover the only behaviour that changes, and the markdownify dependency is unpinned, so "obsolete" depends on the version that gets resolved.
What I checked locally (base 4cc9fa1 -> head 0924b60, markdownify 1.2.3 as resolved here):
- Dispatch. markdownify 1.2.3 builds the hook name from the tag (
convert_%s, markdownify/init.py:385-397) and only falls back to the heading regex at :399-403, which callsconvert_hN(n, el, text, parent_tags). That release has noconvert_hn, so_CustomMarkdownify.convert_hncannot run for h1-h6. The PR description is right that the override is dead code there. - It is not unreachable in every case, though. A literal
<hn>tag produces exactly the nameconvert_hn, and the dispatcher calls it asconvert_fn(node, text, parent_tags=parent_tags)(markdownify/init.py:324). On base that raisesTypeError: _CustomMarkdownify.convert_hn() missing 1 required positional argument: 'text'; the exception is collected by_markitdown.py:669-676and the generic plain-text converter silently takes over. End to end on base,MarkItDown().convert_stream(b"<p>Before</p><hn>Literal hn</hn><p>After</p>")returns the raw HTML'<p>Before</p><hn>Literal hn</hn><p>After</p>'and no error reaches the caller; on head the same input returns'Before\n\nLiteral hn\n\nAfter'. So this is a small bug fix as well as a cleanup. - Corpus. I converted 33 cases through
HtmlConverter(h1-h6, heading with text before/after, headings in lists, tables, divs and 40-deep nesting, ATX / ATX_CLOSED / UNDERLINED, inline markup, and the repo's test_blog.html, test_serp.html, test_wikipedia.html). Base and head are byte-identical except the three<hn>/<HN>cases. - Tests.
packages/markitdown/tests/test_html.pyis 34 passed on base and 44 passed on head (the 10 new cases). I also ran the head file against the base source: 44 passed, so the new cases are behaviour-preservation coverage, not regression coverage. A<hn>case is red on base and green on head (2 failed / 2 passed). - Version range.
packages/markitdown/pyproject.toml:30is the bare string"markdownify"and the repo has no lockfile, so an environment can still be on a release where this hook was live. Forcing markdownify 0.13.1 into the same corpus changes 14 of 33 cases: 0.13.x still exposesconvert_hn(1.0.0 and 1.1.0 renamed it_convert_hn, 1.2.x usesconvert_hN), so base keeps the separator the override added and head does not, e.g. base'Before\n# Heading\n\nAfter'vs head'Before# Heading\n\nAfter', and the three real-world vectors change length too (blog 10903 -> 10898, serp 70208 -> 70191, wikipedia 192118 -> 192086).
What I would like to see before this lands:
- One regression case for the tag that actually dispatches
convert_hn(<hn>, and the uppercase variant). Without it, nothing in the suite fails if that name comes back. - A sentence in the description, or a lower bound in pyproject, making the supported markdownify range explicit, since the cleanup assumes >= 1.0. If older markdownify is not supported, a bound also prevents this class of divergence.
Not in scope, same file and same era mismatch: convert_img still takes convert_as_inline, which the 1.x dispatcher never passes (it sends parent_tags= as a keyword), so that branch is dead. It is latent because upstream only collapses inline images when keep_inline_images_in is set; I confirmed the value received is always False on 1.2.3.
|
Thanks for the thorough review and for catching the I've addressed both points in commit 200660a:
|
Closes #238.
A literal
<hn>or<HN>element accidentally dispatches_CustomMarkdownify.convert_hn, whose signature is incompatible with markdownify's element callbacks. HTML conversion then fails, andMarkItDownfalls back to plain text, returning the original HTML instead of Markdown:With this fix, the output is:
Remove the obsolete hook. Normal
h1–h6rendering already uses markdownify's inherited heading renderer and supplies the needed separators. Requiremarkdownify>=1.0.0: the oldconvert_hnhook was still active in 0.13.x, so removing it without a dependency bound could change normal heading spacing in older installations.Add two end-to-end regression cases for
<hn>and<HN>(both fail against the base implementation) and ten behavior-preservation cases for all six heading levels beside bare text, adjacent headings with ATX/ATX_CLOSED/UNDERLINED styles, and inline emphasis.Validation on macOS ARM64 against upstream
7482c09(October 26, 2026):GITHUB_ACTIONS=true python -m pytest packages/markitdown/tests -q -ra). Skips cover external services, native Windows paths, and optional ExifTool.hatch test --python 3.14 -- -q -ra: 1,130 passed, 57 skipped, with one existingnturl2pathdeprecation warning.pre-commit run --all-files, wheel/sdist builds, wheel dependency metadata, CLI smoke test, andgit diff --check: passed.