feat(update): preserve developer-owned regions across updates (#2184) - #2762
feat(update): preserve developer-owned regions across updates (#2184)#2762DrKat0m wants to merge 6 commits into
Conversation
…-org#2184) Templates often ship files with dummy content or TODO markers that the developer replaces right after generation. Previously `copier update` treated those regions like any other template-owned content, so a change to the placeholder in the template could clobber the developer's work or raise a needless merge conflict. Template authors can now wrap a developer-owned region in literal marker comments that survive rendering: # copier:preserve:start <optional-id> ...developer-owned content... # copier:preserve:end <optional-id> During an update the content between the markers is captured from the current project and injected into every intermediate render (old render, new render, and the freshly rendered destination) so all three merge sides agree on the region. The merge therefore keeps the developer's content conflict-free, while the marker lines and surrounding content keep updating from the template as usual. - Added `copier/_preserve.py` with the marker parsing, capture and restore primitives. - Wired capture/restore into `Worker._apply_update`, scoping the scan to template-managed files discovered in the old render. - Documented the feature in `docs/updating.md`. - Added unit and integration tests in `tests/test_preserve.py`.
|
Hi @sisp, whenever you have a chance, could you please take a look at this PR? I'd really appreciate any feedback on the approach (especially the choice to keep the region conflict free by aligning all three merge sides rather than post processing conflict output) and on the marker syntax. Happy to iterate on anything. Thank you! |
`test_tools.py::test_types` runs `mypy .` over the whole tree and flagged three `_apply_regions` calls in `tests/test_preserve.py`: the local dicts were inferred as `dict[str, str]`, which is not assignable to the expected `dict[_RegionKey, str]` because `dict` is invariant in its key type. Annotate the three test dicts with `_RegionKey` so they match the parameter type.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2762 +/- ##
==========================================
+ Coverage 97.29% 97.32% +0.02%
==========================================
Files 60 61 +1
Lines 7584 7668 +84
==========================================
+ Hits 7379 7463 +84
Misses 205 205
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @sisp, just a gentle follow up whenever you have some time. I'd really appreciate it if you could take a look at this PR and share any feedback you have. In particular, I'd love to know your thoughts on the overall approach and whether the marker syntax feels appropriate. Thank you! |
|
Just a quick thought on this (because I'm on vacation 🌴 ): If the preserve markers must remain in the rendered files (as you document), then they must be valid comments of the respective file format. This means, a hardcoded |
|
Ideally, the markers aren't part of a rendered file, then we could use Jinja syntax and be language-agnostic. Also, use of Copier wouldn't leak into rendered files. Perhaps we could omit developer-owned sections from old and new copies that are created by the update algorithm? This way, the 3-way merge should ignore changes in the template in those sections. #2376 might make this even easier. We could try this quickly by exporting {%- if _copier_operation != 'update' %}
...
{%- endif %}If this works, I'd prefer a custom tag like {%- ignore %}
...
{%- endignore %}for better DX. |
…opier-org#2184) Replaced the literal comment-marker approach with a language-agnostic Jinja `{% ignore %}` / `{% endignore %}` tag. The block renders on `copier copy` but is omitted from the renders Copier produces internally during `copier update`, so its content stays developer-owned while no Copier syntax leaks into rendered files. Because the region is absent from both the old and new template renders that feed the 3-way merge, template changes inside it never reach the diff and the developer's own version survives, degrading to a normal Copier conflict only when the template edits lines directly adjacent to the block. - Added `IgnoreExtension`, compiling the tag to the equivalent of `{% if _copier_operation != 'update' %}...{% endif %}`. - Expose `_copier_operation` in the file render context and merge it with per-call extra context to avoid duplicate-keyword render errors. - Removed `copier/_preserve.py` and its `_apply_update` wiring. - Updated the `updating.md` docs and replaced the tests.
134472f to
f55c390
Compare
|
Thanks for the feedback - enjoy the vacation! 🌴 You're right that hardcoded comment markers can't work across an open set of formats, so I reworked this the way you suggested. Dropped the hardcoded markers in favor of a Jinja tag - {% ignore -%} It's a small extension compiling to {% if _copier_operation != 'update' %}…{% endif %} - rendered on copier copy, omitted from the internal renders during copier update. Being pure Jinja, nothing leaks into rendered files and it's fully language agnostic. However, since the region is omitted from both merge sides, the developer's content re-applies as a diff hunk, clean when there's stable context around the block, but if the template edits a line directly adjacent to it, it degrades to a normal merge conflict. |
|
Hi @sisp, just a gentle follow up whenever you have some time. I'd really appreciate it if you could take a look at this PR and share any feedback you have. |
| else: | ||
| new_content = tpl.render( | ||
| **self._render_context(), **(extra_context or {}) | ||
| {**self._render_context(), **(extra_context or {})} |
There was a problem hiding this comment.
This is diff noise, not strictly needed to implement the new feature.
| {**self._render_context(), **(extra_context or {})} | |
| **self._render_context(), **(extra_context or {}) |
| """ | ||
| tpl = self.jinja_env.from_string(string) | ||
| return tpl.render(**self._render_context(), **(extra_context or {})) | ||
| return tpl.render({**self._render_context(), **(extra_context or {})}) |
There was a problem hiding this comment.
This is diff noise, not strictly needed to implement the new feature.
| return tpl.render({**self._render_context(), **(extra_context or {})}) | |
| return tpl.render(**self._render_context(), **(extra_context or {})) |
| !!! tip "Nothing leaks into the rendered file" | ||
|
|
||
| Unlike a comment-based marker, the `{% ignore %}` tag is pure Jinja — it is stripped | ||
| during rendering, so no Copier-specific syntax ends up in your generated project. It | ||
| is therefore **language-agnostic**: it works in `.js`, `.go`, `.c`, `.rs` … or any | ||
| other text file, since it never has to be a valid comment in the target language. | ||
|
|
There was a problem hiding this comment.
I think this is pretty obvious, let's try keeping the docs concise. I presume AI was in a chatty mood. 😄
| !!! tip "Nothing leaks into the rendered file" | |
| Unlike a comment-based marker, the `{% ignore %}` tag is pure Jinja — it is stripped | |
| during rendering, so no Copier-specific syntax ends up in your generated project. It | |
| is therefore **language-agnostic**: it works in `.js`, `.go`, `.c`, `.rs` … or any | |
| other text file, since it never has to be a valid comment in the target language. |
| The tag relies on the `_copier_operation` render context variable, which is `"copy"` | ||
| during generation and `"update"` during an update. If you prefer to be explicit, or need | ||
| a condition the tag can't express, you can write the equivalent directly: | ||
|
|
||
| ```jinja | ||
| {% if _copier_operation != "update" %} | ||
| ...developer-owned content... | ||
| {% endif %} | ||
| ``` |
There was a problem hiding this comment.
I'm not sure we should include this snippet, I don't quite see when anybody would want to use the explicit condition, so including it might suggest this is a reasonably common scenario.
| The tag relies on the `_copier_operation` render context variable, which is `"copy"` | |
| during generation and `"update"` during an update. If you prefer to be explicit, or need | |
| a condition the tag can't express, you can write the equivalent directly: | |
| ```jinja | |
| {% if _copier_operation != "update" %} | |
| ...developer-owned content... | |
| {% endif %} | |
| ``` |
| !!! important "Leave stable context around the block" | ||
|
|
||
| The `-%}` and `{%-` [whitespace-control][whitespace] markers strip the tag's own | ||
| lines so the output stays clean. Keep at least one unchanging line — such as the | ||
| blank lines above — directly around the block, and avoid editing the lines | ||
| immediately adjacent to it in later template versions. The update re-applies your | ||
| region as a diff against the surrounding lines; if the template changes a line right | ||
| next to the block, the merge falls back to a normal Copier | ||
| [conflict](#recover-from-a-broken-update) that you resolve by hand (your content is | ||
| never silently lost). Embedding the block among stable code, rather than at the very | ||
| top or bottom of a tiny file, gives the merge the anchors it needs. |
There was a problem hiding this comment.
I'm not sure we should phrase this as a recommendation because users may not be able to avoid touching adjacent lines that are part of the hunk context. It might be better to inform users about the behavior, so they aren't surprised when merge conflicts involving the ignored block occur.
| def test_copy_renders_block_without_leaking_syntax( | ||
| tmp_path_factory: pytest.TempPathFactory, | ||
| ) -> None: | ||
| """Initial generation renders the block; no marker/tag survives.""" | ||
| src, dst = map(tmp_path_factory.mktemp, ("src", "dst")) | ||
| _commit_template(src, _V1, "v1") | ||
|
|
||
| run_copy(str(src), dst, defaults=True, overwrite=True, vcs_ref="v1") | ||
|
|
||
| rendered = (dst / "app.py").read_text(encoding="utf-8") | ||
| assert 'return "dummy v1"' in rendered # scaffolding is present after copy | ||
| assert "ignore" not in rendered # no Copier syntax leaks into the file | ||
| assert "{%" not in rendered and "%}" not in rendered |
There was a problem hiding this comment.
I'd omit this test because the (updated version of the) tests below also covers this implicitly.
| # Unit tests for the extension in isolation. | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def env() -> SandboxedEnvironment: | ||
| return SandboxedEnvironment(extensions=[IgnoreExtension]) | ||
|
|
||
|
|
||
| def test_ignore_renders_body_outside_update(env: SandboxedEnvironment) -> None: | ||
| template = env.from_string("a\n{% ignore %}b{% endignore %}\nc") | ||
| assert template.render({"_copier_operation": "copy"}) == "a\nb\nc" | ||
|
|
||
|
|
||
| def test_ignore_omits_body_on_update(env: SandboxedEnvironment) -> None: | ||
| template = env.from_string("a\n{% ignore %}b{% endignore %}\nc") | ||
| assert template.render({"_copier_operation": "update"}) == "a\n\nc" | ||
|
|
||
|
|
||
| def test_ignore_trim_markers_clean_output(env: SandboxedEnvironment) -> None: | ||
| """The idiomatic ``-%}``/``{%-`` markers strip the tag lines entirely.""" | ||
| template = env.from_string("a\n{% ignore -%}\nb\n{%- endignore %}\nc") | ||
| assert template.render({"_copier_operation": "copy"}) == "a\nb\nc" | ||
| assert template.render({"_copier_operation": "update"}) == "a\n\nc" |
There was a problem hiding this comment.
I'm not sure we need these tests. In the end, what really matters is the behavior involving run_copy and run_update.
| def _commit_template(src: Path, body: str, tag: str) -> None: | ||
| build_file_tree( | ||
| { | ||
| src / "{{ _copier_conf.answers_file }}.jinja": ( | ||
| "{{ _copier_answers|to_nice_yaml }}\n" | ||
| ), | ||
| src / "app.py.jinja": body, | ||
| }, | ||
| dedent=False, | ||
| ) | ||
| with local.cwd(src): | ||
| git("init") if not (src / ".git").exists() else None | ||
| git("add", "-A") | ||
| git("commit", "-m", tag) | ||
| git("tag", tag) | ||
|
|
||
|
|
||
| _V1 = """\ | ||
| import os | ||
| CONFIG = "v1" | ||
|
|
||
|
|
||
| def stable_helper(): | ||
| return 42 | ||
|
|
||
|
|
||
| {% ignore -%} | ||
| def user_code(): | ||
| return "dummy v1" | ||
| {%- endignore %} | ||
|
|
||
|
|
||
| def another_stable(): | ||
| return "keep" | ||
| """ | ||
|
|
||
| _V2 = ( | ||
| _V1.replace('CONFIG = "v1"', 'CONFIG = "v2"') | ||
| .replace('return "dummy v1"', 'return "dummy v2"') | ||
| .replace('return "keep"', 'return "kept in v2"') | ||
| ) |
There was a problem hiding this comment.
I think it would be more idiomatic to create a fixture that creates the test template with two commits/tags rather than having a helper function and constants. For brevity and convenience, you can use the git_save helper function instead of the git init/add/commit/tag command sequence.
| assert "<<<<<<<" not in result # clean merge | ||
| assert 'return "REAL IMPLEMENTATION"' in result # developer content kept | ||
| assert 'return "dummy v2"' not in result # template placeholder ignored | ||
| assert 'CONFIG = "v2"' in result # surrounding code updated | ||
| assert 'return "kept in v2"' in result | ||
| assert "ignore" not in result # still no leaked syntax |
There was a problem hiding this comment.
I think it would be simpler and more explicit to assert the full file content. You can use inline-snapshot to ease snapshot testing:
| assert "<<<<<<<" not in result # clean merge | |
| assert 'return "REAL IMPLEMENTATION"' in result # developer content kept | |
| assert 'return "dummy v2"' not in result # template placeholder ignored | |
| assert 'CONFIG = "v2"' in result # surrounding code updated | |
| assert 'return "kept in v2"' in result | |
| assert "ignore" not in result # still no leaked syntax | |
| assert result == snapshot() |
For this, add from inline_snapshot import snapshot at the top. When you run uv run pytest -n0 --inline-snapshot=review ..., you'll be prompted to review the snapshot content and it'll be inserted for you if you agree.
Same applies to the other test below.
| src, dst = map(tmp_path_factory.mktemp, ("src", "dst")) | ||
| _commit_template(src, _V1, "v1") | ||
|
|
||
| run_copy(str(src), dst, defaults=True, overwrite=True, vcs_ref="v1") |
There was a problem hiding this comment.
Perhaps we can add an assertion that checks the content of app.py here as well. See my comment about snapshot testing below.
Templates often ship files with dummy content or TODO markers that the developer replaces right after generation. Previously
copier updatetreated those regions like any other template-owned content, so a change to the placeholder in the template could clobber the developer's work or raise a needless merge conflict.Template authors can now wrap a developer-owned region in literal marker comments that survive rendering:
During an update the content between the markers is captured from the current project and injected into every intermediate render (old render, new render, and the freshly rendered destination) so all three merge sides agree on the region. The merge therefore keeps the developer's content conflict-free, while the marker lines and surrounding content keep updating from the template as usual.
copier/_preserve.pywith the marker parsing, capture and restore primitives.Worker._apply_update, scoping the scan to template-managed files discovered in the old render.docs/updating.md.tests/test_preserve.py.Closes #2184