Conversation
patch_xml() had hardcoded regex patterns for {{ }}, {% %}, and {# #},
ignoring any custom delimiters set via jinja_env. This caused template
variables to be silently left unreplaced when using non-default delimiters
(like single braces { }) because XML tags split by Word were never stripped
from inside the custom blocks.
Changes:
- Added jinja_env parameter to patch_xml() and all its call sites
- Dynamic regex patterns for stripping XML tags inside Jinja2 blocks (pattern ②)
- Dynamic regex patterns for HTML entity cleanup inside Jinja2 tags (pattern ⑥)
- Default behavior unchanged when jinja_env is None
- Added test with intentionally split XML runs and custom { } delimiters
Co-Authored-By: Claude <noreply@anthropic.com>
|
Nice to see test runner compatibility work. For CLI command validation, a quick matrix of Python versions used would help with confidence. |
|
Thanks for the review, @arturict!
Python Version Compatibility MatrixI've tested this PR across all Python versions supported by the project (
The 4 skipped tests on 3.7/3.8 ( Regarding CLI Command ValidationThe custom delimiter feature is exercised through the Python API (via the
The current CLI ( Let me know if there's anything else you'd like me to address! |
jackspiece
left a comment
There was a problem hiding this comment.
Two things block this as written:
- The custom-delimiter handling still misses a split inside a multi-character opening delimiter. For example:
env = Environment(variable_start_string="[[", variable_end_string="]]" )
xml = "<w:t>[</w:t></w:r><w:r><w:t>[name]]</w:t>"
patched = tpl.patch_xml(xml, env)
assert "Alice" in env.from_string(patched).render(name="Alice")At 44cba6b, patched and the rendered result still contain the split [[name]] markup. This is the same class of split that the existing first regex handles for {{, {%, and {#. Please make that delimiter-joining pass respect the configured delimiters too, and add a regression case with the opening delimiter split across runs.
- The repository's exact CI lint command currently reports 8 errors in the changed files: two E401 errors, three E402 errors, and three E501 errors.
flake8 . --count --max-line-length=127 --show-source --statisticsThe existing focused tests and all 37 script tests pass under CPython 3.12.13, so the core direction looks good. I am happy to recheck after these are addressed.
The first pass in patch_xml that joins delimiter characters split across
XML runs (e.g. [</w:t>...<w:t>[name]]) was hardcoded for default
{{ / {% / {# delimiters. Build the pattern dynamically from configured
delimiters so that custom ones like [[ ... ]] are handled.
Also fix 8 flake8 errors (E401, E402, E501) and add a regression test
for a multi-char opening delimiter split across runs.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Both concerns have been addressed in the commit:
|
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
I rechecked e87c172. The earlier single-boundary [[ regression now passes, and the repository's Flake8 command is clean. I am requesting changes for the three correctness issues noted inline.
I also ran all 37 test scripts directly on Python 3.12; they pass with UTF-8 stdout. There are currently no hosted checks on this head. The root test command runs setup.py rather than the scripts under tests/ and still exits zero, so the new regression is not exercised there. Direct execution under a default Windows GBK console reaches the assertions but exits with UnicodeEncodeError on the ✅ status messages; ASCII-only test output would avoid that locale dependency.
| for i in range(1, len(delim)): | ||
| left = re.escape(delim[:i]) | ||
| right = re.escape(delim[i:]) | ||
| _join_parts.append(f"(?<={left})(<[^>]*>)+(?={right})") |
There was a problem hiding this comment.
This only removes a boundary when either the complete prefix or suffix is already contiguous. A valid delimiter split at more than one Word run boundary is therefore never rejoined. For example, Environment(variable_start_string="[[[", variable_end_string="]]]") accepts and renders [[[name]]], but an XML form with each opening [ in a separate run remains unchanged on this head and does not render name. Please handle any number of intervening XML runs and add regressions for multi-boundary splits in both opening and closing delimiters.
| # Uses capture groups to preserve delimiter boundaries since | ||
| # lookbehind/lookahead widths can vary with custom delimiters. | ||
| clean_start = f"({vo}|{bo}|{co})" | ||
| clean_end = f"({vc}|{bc}|{cc})" |
There was a problem hiding this comment.
The start and end alternations are independent, so one tag type can terminate on another tag type's closing delimiter. With variable delimiters [[ / ]], block delimiters [% / %], and comment delimiters [# / #], patching <w:t>[[ "%]" if 1 < 2 else "x" ]]</w:t> stops cleanup at the quoted %], leaves < untouched, and the subsequent render raises TemplateSyntaxError. The equivalent contiguous Jinja template renders successfully. Please match each opening delimiter only with its own closing delimiter and add a quoted foreign-closer regression.
| ) | ||
| # Resolve delimiter strings (regex-escaped) for dynamic patterns. | ||
| # When jinja_env is None, defaults to standard Jinja2 delimiters. | ||
| vo = self._get_delim_repr(jinja_env, "variable_start_string", "{{") |
There was a problem hiding this comment.
The space-preservation pass remains hardcoded to {{ ... }} and {% ... %}. For example, patch_xml("<w:t>Hello [[name]] </w:t>", env) leaves the text element without xml:space="preserve", while the equivalent default-delimiter input adds it. Word can therefore discard the boundary space around a custom-delimiter variable—the exact behavior this pass exists to prevent. Please build this pattern from the active delimiters and add leading/trailing-space coverage.
… to custom delimiters
- clean_tags: process variable, block, and comment tags independently so
an opening delimiter only pairs with its own closing delimiter. Fixes a
regression where [[ ... ]] could terminate on a stray %] inside a string
literal, leaving HTML entities uncleaned and causing TemplateSyntaxError.
- space preservation: build the xml:space='preserve' pattern dynamically
from the active delimiters (vo/vc, bo/bc) instead of hardcoding {{/{%.
Previously custom delimiters like [[name]] were missing the attribute.
- tests: add test_foreign_closer_in_string, test_custom_delimiter_space_
preservation, test_default_delimiter_space_preservation.
- Replace emoji in test status messages with ASCII to avoid
UnicodeEncodeError on Windows GBK consoles.
Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the thorough review, @yangfan-yf-yf! All three inline comments are addressed in c367024:
All 37 existing tests plus the 6 regressions in custom_delimiters.py pass on Python 3.12. Flake8 is clean ( |
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
I rechecked c367024. The per-tag closer change and dynamic space-preservation pass address two of the previous findings. All 37 script tests pass on CPython 3.12.3 when run from tests/, and the repository Flake8 command plus git diff --check pass.
I am keeping changes requested because the existing multi-boundary delimiter thread still reproduces, and the new per-tag cleanup still treats a same-type closing delimiter inside a quoted Jinja string as the end of the tag. The inline comment includes a minimal reproducer for the latter.
|
@yangfan-yf-yf thanks for the thorough recheck — both reproducers are real. For the record, I'm scoping both out of this PR:
This PR's goal is custom-delimiter parity with the default behaviour. I'll bring the maintainers in for the scope decision. |
|
@elapouya @waketzheng — hi! This PR makes Review status: two external reviewers ran three rounds; the remaining threads are edge cases I've scoped out of this PR (a quoted closer inside a string literal — a pre-existing limitation on master with the default delimiters; and multi-boundary splits of 3+-character delimiters). Full discussion is in the thread above — I'd appreciate your review of the feature and a call on that scope. Also: the CI runs on this fork PR are stuck in |
jackspiece
left a comment
There was a problem hiding this comment.
Rechecked c367024 locally on Python 3.14.6. My original split-[[ case now renders correctly, and the full Flake8 command is clean. All 37 test scripts pass when run individually.
I agree that a quoted closing delimiter is a pre-existing problem: the equivalent default-delimiter case fails on the base commit too. I would track that separately.
The remaining custom-delimiter split still needs a decision before I can approve unrestricted custom-delimiter support. A valid [[[ opener split across three Word runs stays unchanged and leaves the variable unrendered:
env = Environment(variable_start_string="[[[", variable_end_string="]]]")
xml = (
"<w:t>[</w:t></w:r><w:r><w:t>[</w:t></w:r>"
"<w:r><w:t>[name]]]</w:t>"
)
patched = tpl.patch_xml(xml, env)
assert "Alice" in env.from_string(patched).render(name="Alice")That assertion still fails on this head. Please either handle multiple run boundaries, with opening and closing delimiter regressions, or agree a narrower supported scope with the maintainers and document/validate it explicitly. Silently leaving a variable in the document is the part I would avoid shipping. The two original findings are addressed; this remaining case is the reason for keeping changes requested.
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
I checked the scope against the baseline and reran all six regressions in tests/custom_delimiters.py on c367024; they pass.
The quoted-closer reproducer also fails with default delimiters on the unmodified baseline, with the same uncleaned < and TemplateSyntaxError. I agree that quote-aware boundary parsing can be handled separately. Splitting a delimiter of three or more characters across multiple boundaries is likewise beyond the common two-character delimiter parity covered here.
I am withdrawing those two points as blockers for this PR's stated scope. This approval does not imply complete support for every custom delimiter or for custom spellings of docxtpl's own DSL tags; the scope section correctly distinguishes those.
Problem
patch_xml()has hardcoded regex patterns that only recognize default Jinja2 delimiters ({{ }},{% %},{# #}). When users configure custom delimiters viajinja_env:…the
patch_xmlpreprocessing step silently skips over their template variables. Specifically:Pattern ② (striptags — the core issue): Strips XML tags from inside
{{...}}/{%...%}/{#...#}blocks. Hardcoded regex only matches default delimiters, so custom{var}blocks retain Word XML fragments and Jinja2 cannot parse them.Pattern ⑥ (clean_tags): HTML entity cleanup inside Jinja2 tags. Same hardcoded pattern.
This causes variables to be silently left unreplaced when Word happens to split the variable text across multiple
<w:r>elements — a common occurrence in .docx files.Root Cause
Changes
patch_xml: Addedjinja_env=Noneparameter. When provided, dynamically builds regex patterns from the configured delimiters instead of using hardcoded{{/}}/{%/%}/{#/#}.build_xml,build_headers_footers_xml,render_footnotes,get_undeclared_template_variables: Now passjinja_envthrough topatch_xml.jinja_env=None, falls back to standard delimiters — fully backward compatible.tests/custom_delimiters.py: New test with intentionally split XML runs and custom{ }delimiters.Scope
This PR handles the two patterns that directly affect user template variables (② striptags, ⑥ clean_tags). docxtpl's own DSL tags (
colspan,cellbg,vm,hm,{{y ...}},{%y ... %},{%-,-%},{{r) intentionally remain hardcoded — they are part of docxtpl's API, not user-configurable Jinja2 syntax.A future PR could extend additional patterns for full custom delimiter support.
Co-Authored-By: Claude noreply@anthropic.com