Handle escaped markdown markers inside markdown links - #1883
Handle escaped markdown markers inside markdown links#1883apoorvdarshan wants to merge 2 commits into
Conversation
|
Thanks for working on this. I think this fixes the narrower escaping symptom from the issue, but I have a scope concern before we close #1847. In the issue discussion, the agreed scoped behavior was to support markdown markers that both open and close inside the link label, for example: "[**Issue** 1847](url)"should render Issue 1847 as bold while keeping the whole label linked. We explicitly wanted to avoid supporting markers that span across the link boundary, but markers fully contained inside the link text were expected to be parsed. This PR instead keeps the existing behavior where markers inside link labels are always literal. So Issue still renders visible Issue, and \Issue\ now converges to the same literal output. That solves the stray backslash symptom, but not the behavior discussed in #1847. Could you either:
Also, the CI lint step is failing because of the new protected-member accesses in test/text/test_multi_cell_markdown.py: |
Markdown markers inside a link's display text are intentionally not interpreted, but a preceding escape backslash was previously left untouched. As a result, escaping a marker inside a link (e.g. `[\**Issue\**](url)`) leaked literal backslashes into the rendered text, and the LINES re-serialization emitted the display text verbatim, producing double-escaped output. Escape sequences inside link text are now collapsed with the same rules as `_parse_chars` uses elsewhere (markers kept literal, backslashes consumed), and the LINES re-serialization now escapes the link display text so the round-trip stays stable. Fixes py-pdf#1847.
f0337bf to
eebd2ef
Compare
|
Rebased onto current |
|
Pylint is still failing: |
|
Addressed both review points. Balanced markdown markers that open and close within a link label are now parsed, with the link preserved on every resulting fragment; unbalanced markers remain literal so styling cannot span the link boundary. Escaped markers still render literally without leaking backslashes, and LINES round-tripping remains stable. I also removed the protected-member accesses from test_multi_cell_markdown.py. Verification: 40 focused tests pass, Black passes, and pylint reports 10.00/10. |
andersonhc
left a comment
There was a problem hiding this comment.
Thanks for working on this. I like the direction of making escaped markers inside link labels round-trip correctly.
I’m a bit concerned about _markdown_escape_unbalanced_link_markers(), though. It duplicates part of the marker-recognition logic from _parse_chars(), and the duplicate scanner does not apply all of the same rules. In particular, _parse_chars() ignores markers in some adjacency cases, while the new helper still counts them as balanced.
For example, on this branch:
FPDF()._parse_chars("[***a**b](url)", True)ends up rendering b as bold inside the link. I would expect that label to remain literal, since there is no valid balanced ... pair under the parser’s actual marker rules.
Could we avoid having a second marker recognizer here? I think this would be safer if the marker tuple and “is this marker active at this position?” logic were shared with _parse_chars(), or if the link-label handling reused _parse_chars() in a way that does not need a separate approximation of its rules.
A regression test for cases like *ab or a*b would also help lock this down.
Could you also explore extracting the shared marker-scanning logic between _parse_chars() and _markdown_escape_unbalanced_link_markers() into reusable helper methods?
Right now the new helper duplicates some of _parse_chars()’ marker recognition rules, but not all of them. That makes it easy for the two paths to drift, as in cases where _parse_chars() ignores a marker due to adjacency but _markdown_escape_unbalanced_link_markers() still counts it as balanced.
Having one shared implementation for “is this an active markdown marker here?” and possibly for escape-run handling would make this easier to reason about and would reduce the risk of future parser differences.
Summary
Fixes #1847.
Markdown markers inside the display text of a markdown link are intentionally not interpreted (this is existing, tested behavior —
[**bold**](url)keeps**literal). However, a preceding escape backslash was previously left untouched inside links, unlike everywhere else. As a result:[\**Issue\**](url)) leaked literal backslashes into the rendered text — producing\**Issue\**instead of**Issue**.LINESre-serialization side, the display text was emitted verbatim (aNOTEin the code acknowledged this), so markers inside a link were not escaped while text outside links was, yielding double-escaped output on the round-trip.This is the exact scenario from the issue:
previously rendered the second line with visible escape backslashes.
Fix
FPDF._markdown_unescape_link_text(), which collapses escape sequences inside a link's display text using the same rules as_parse_chars(markdown markers kept literal, escape backslashes consumed), and applied it where the linkFragmentis created.LINESre-serialization to run the link display text through the existingescape()helper, so markers it contains are re-parsed as literal characters and the round-trip stays stable. The staleNOTEcomment was replaced accordingly.Style markers inside links remain literal (unchanged, deliberate behaviour); this PR is scoped strictly to removing the escape/double-escape defect.
Tests
test_markdown_parse_escaped_markers_inside_link(parse side): escaped markers inside a link now consume the backslash and stay literal; doubled backslashes collapse to one — matching outside-link behaviour.test_multi_cell_markdown_escaped_markers_inside_link(LINESround-trip): the parsed fragment and theLINESre-serialization are free of stray escape characters and round-trip stably.Both tests fail on
masterbefore the change and pass after. The fulltest/text/andtest/html/suites pass, and the existing markdown reference-PDF tests (includingtest_multi_cell_markdown_styled_linkand..._dry_run_lines_output_escape) are unchanged, confirming no rendered output regressions.black,pylint, andmypyare clean on the changed files.Disclosure: prepared with AI assistance; reviewed and verified locally.