Skip to content

fix: allow % and } in {%p %}/{%tr %}/{%tc %}/{%r %} expressions - #645

Open
Noethix55555 wants to merge 2 commits into
elapouya:masterfrom
Noethix55555:fix/patch-xml-expr-modulo-brace
Open

fix: allow % and } in {%p %}/{%tr %}/{%tc %}/{%r %} expressions#645
Noethix55555 wants to merge 2 commits into
elapouya:masterfrom
Noethix55555:fix/patch-xml-expr-modulo-brace

Conversation

@Noethix55555

Copy link
Copy Markdown

Fixes #644.

patch_xml matched the expression body of these directive tags with [^}%]*, which stops at the first % or }. Tags whose expression contains a modulo (if i % 2 == 0) or a literal brace (set d = {"x":1}) were not stripped of their surrounding <w:p>/<w:tr>/<w:tc>/<w:r> wrapper, so the literal {%p ...%} reached Jinja and raised TemplateSyntaxError: Encountered unknown tag 'p'.

The fix matches up to the closing %}/}} instead of excluding those characters:

([^}%%]*(?:%%}|}}))  ->  ((?:(?!%%}|}}).)*(?:%%}|}}))

Verified with a real render: a template using {%p if i % 2 == 0 %} raises TemplateSyntaxError on the current code and renders correctly with the fix. The representative existing fixture tests (dynamic_table, nested_for, vertical_merge, horizontal_merge, less_cells_after_loop, comments, escape, merge_paragraph, richtext, order and others) still pass.

CI runs flake8 only and the existing tests are binary .docx fixtures with no assertions, so I did not add a fixture; the reproduction above is in the linked issue. Happy to add a fixture-based test if you prefer.

patch_xml matched the expression body of these directive tags with
[^}%]*, which stops at the first % or }. Tags whose expression contains a
modulo (if i % 2 == 0) or a literal brace (set d = {"x":1}) were not
stripped of their surrounding <w:p>/<w:tr>/<w:tc>/<w:r> wrapper, so the
literal {%p ...%} reached Jinja and raised TemplateSyntaxError:
Encountered unknown tag 'p'.

Match up to the closing %} or }} instead of excluding the characters.
@waketzheng

Copy link
Copy Markdown
Contributor

It would be better if you could add tests.

Add a regression test for patch_xml() relocating {%p %}/{%tr %} directives
whose Jinja expression contains a "%" (modulo / literal percent) or a "}"
(dict literal). These previously broke the relocation regex.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@arturict

Copy link
Copy Markdown

Please include minimal example for new --validate option and expected user-facing messages.

@JackSpiece JackSpiece left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change fixes the provided modulo and single-brace cases, but the new expression still lets either opener terminate on either closer. A {%p ... %} block can therefore stop at }}, even though only %} should close it.

Here is a full DOCX reproduction at 2c534f0:

from docx import Document
from docxtpl import DocxTemplate

doc = Document()
doc.add_paragraph("{%p set d = {'a': {}} %}")
doc.add_paragraph("{{ d }}")
doc.save("nested-dict.docx")

tpl = DocxTemplate("nested-dict.docx")
tpl.render({})

This raises TemplateSyntaxError. The equivalent plain Jinja template renders {'a': {}} correctly. At the string-transform level, patch_xml() truncates the directive to {% set d = {'a': {}} and drops its %}.

Please pair {% only with %} and {{ only with }}, then add a nested-dictionary regression case. The new focused test, all 37 repository scripts under CPython 3.12.13, and the exact flake8 command otherwise pass.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

{%p %} and similar tags fail when the expression contains % or }

4 participants