Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add _copier_conf.operation variable #1733

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions copier/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@
MISSING,
AnyByStrDict,
JSONSerializable,
Operation,
RelativePath,
Self,
StrOrPath,
)
from .user_data import DEFAULT_DATA, AnswersMap, Question
Expand Down Expand Up @@ -205,6 +207,7 @@ class Worker:
skip_tasks: bool = False

answers: AnswersMap = field(default_factory=AnswersMap, init=False)
operation: Operation = field(init=False, default="copy")
_cleanup_hooks: list[Callable[[], None]] = field(default_factory=list, init=False)

def __enter__(self) -> Worker:
Expand Down Expand Up @@ -242,7 +245,7 @@ def _cleanup(self) -> None:
for method in self._cleanup_hooks:
method()

def _check_unsafe(self, mode: Literal["copy", "update"]) -> None:
def _check_unsafe(self, mode: Operation) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass ˋmodeˋ if it is in ˋself.operationˋ, right?

Copy link
Contributor Author

@lkubb lkubb Aug 16, 2024

Choose a reason for hiding this comment

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

Initially I did it like that, but noticed that this would cause a behavior change (at least in theory):

During _apply_update(), self.operation is update, but it calls on run_copy() several times, which would pass copy to _check_unsafe() before this patch.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. However, that is correct. You will notice that there are calls to replace. In those calls, you can replace some configuration for the sub-worker that is created. Could you please try doing it that way?

Copy link
Contributor Author

@lkubb lkubb Sep 22, 2024

Choose a reason for hiding this comment

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

Sorry, I'm not sure I'm following. First, let me sum up:

  • Introducing _copier_conf.operation means we have an attribute on the worker representing the current high-level (user-requested) operation.
  • You're proposing to use this reference for _check_unsafe instead of the parameter.
  • I noted that doing this will change how _check_unsafe behaves during the individual copy operations that run during an update, where the high-level operation is update, but the low-level one is copy, advocating for keeping the parameter.

I'm already using replace for overriding the operation during update. Are you saying the high-level operation during the individual copy operations should be copy? Because that would mean _copier_conf.operation is always copy during template rendering, i.e. defeat the purpose of this feature.

"""Check whether a template uses unsafe features."""
if self.unsafe:
return
Expand Down Expand Up @@ -587,7 +590,7 @@ def _pathjoin(
@cached_property
def match_exclude(self) -> Callable[[Path], bool]:
"""Get a callable to match paths against all exclusions."""
return self._path_matcher(self.all_exclusions)
return self._path_matcher(map(self._render_string, self.all_exclusions))

@cached_property
def match_skip(self) -> Callable[[Path], bool]:
Expand Down Expand Up @@ -809,6 +812,14 @@ def template_copy_root(self) -> Path:
subdir = self._render_string(self.template.subdirectory) or ""
return self.template.local_abspath / subdir

def replace(self, **kwargs: Any) -> Self:
"""Wraps dataclasses.replace to support replacing the init=False operation field."""
operation = kwargs.pop("operation", self.operation)
new = replace(self, **kwargs)
if operation in ("copy", "update"):
new.operation = operation
return new

# Main operations
def run_copy(self) -> None:
"""Generate a subproject from zero, ignoring what was in the folder.
Expand Down Expand Up @@ -854,7 +865,7 @@ def run_recopy(self) -> None:
"Cannot recopy because cannot obtain old template references "
f"from `{self.subproject.answers_relpath}`."
)
with replace(self, src_path=self.subproject.template.url) as new_worker:
with self.replace(src_path=self.subproject.template.url) as new_worker:
new_worker.run_copy()

def run_update(self) -> None:
Expand Down Expand Up @@ -904,8 +915,10 @@ def run_update(self) -> None:
print(
f"Updating to template version {self.template.version}", file=sys.stderr
)
self._apply_update()
self._print_message(self.template.message_after_update)
with self.replace(operation="update") as worker:
worker._apply_update()
worker._print_message(worker.template.message_after_update)
self.answers = worker.answers

def _apply_update(self) -> None: # noqa: C901
git = get_git()
Expand All @@ -927,8 +940,7 @@ def _apply_update(self) -> None: # noqa: C901
prefix=f"{__name__}.dst_copy.",
) as dst_copy:
# Copy old template into a temporary destination
with replace(
self,
with self.replace(
dst_path=old_copy / subproject_subdir,
data=self.subproject.last_answers,
defaults=True,
Expand Down Expand Up @@ -990,8 +1002,7 @@ def _apply_update(self) -> None: # noqa: C901
with suppress(AttributeError):
del self.subproject.last_answers
# Do a normal update in final destination
with replace(
self,
with self.replace(
# Don't regenerate intentionally deleted paths
exclude=exclude_plus_removed,
# Files can change due to the historical diff, and those
Expand All @@ -1003,10 +1014,9 @@ def _apply_update(self) -> None: # noqa: C901
current_worker.run_copy()
self.answers = current_worker.answers
# Render with the same answers in an empty dir to avoid pollution
with replace(
self,
with self.replace(
dst_path=new_copy / subproject_subdir,
data=self.answers.combined, # type: ignore[arg-type]
data=self.answers.combined,
defaults=True,
quiet=True,
src_path=self.subproject.template.url, # type: ignore[union-attr]
Expand Down
6 changes: 6 additions & 0 deletions copier/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
else:
from typing_extensions import Annotated

if sys.version_info >= (3, 11):
from typing import Self as Self
else:
from typing_extensions import Self as Self

# simple types
StrOrPath = Union[str, Path]
AnyByStrDict = Dict[str, Any]
Expand All @@ -40,6 +45,7 @@
Env = Mapping[str, str]
MissingType = NewType("MissingType", object)
MISSING = MissingType(object())
Operation = Literal["copy", "update"]


# Validators
Expand Down
14 changes: 14 additions & 0 deletions docs/configuring.md
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,18 @@ to know available options.

The CLI option can be passed several times to add several patterns.

Each pattern can be templated using Jinja.

!!! example

Templating `exclude` patterns using `_copier_conf.operation` allows to have files
that are rendered once during `copy`, but are never updated:

```yaml
_exclude:
- '{%- if _copier_conf.operation == "update" -%}src/*_example.py{%- endif %}'
```

!!! info

When you define this parameter in `copier.yml`, it will **replace** the default
Expand Down Expand Up @@ -1316,6 +1328,8 @@ configuring `secret: true` in the [advanced prompt format][advanced-prompt-forma
exist, but always be present. If they do not exist in a project during an `update`
operation, they will be recreated.

Each pattern can be templated using Jinja.

!!! example

For example, it can be used if your project generates a password the 1st time and
Expand Down
3 changes: 3 additions & 0 deletions docs/creating.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,6 @@ Copier includes:
- It contains the current commit hash from the template in
`{{ _copier_conf.vcs_ref_hash }}`.
- Contains Operating System-specific directory separator under `sep` key.
- It also contains the current `operation` (`copy` | `update`). This value is
intended to be used for templating Copier configuration. It cannot be used in
templated project paths and files.
2 changes: 1 addition & 1 deletion poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pydantic = ">=2.4.2"
pygments = ">=2.7.1"
pyyaml = ">=5.3.1"
questionary = ">=1.8.1"
typing-extensions = { version = ">=3.7.4,<5.0.0", python = "<3.9" }
typing-extensions = { version = ">=4.0.0,<5.0.0", python = "<3.11" }
eval-type-backport = { version = ">=0.1.3,<0.3.0", python = "<3.10" }

[tool.poetry.group.dev]
Expand Down
77 changes: 77 additions & 0 deletions tests/test_context.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import pytest
from plumbum import local

import copier

from .helpers import build_file_tree, git_save


@pytest.mark.parametrize("operation", ("recopy", "update"))
def test_operation_in_context_matches(
operation: str, tmp_path_factory: pytest.TempPathFactory
) -> None:
"""
Ensure that the _copier_conf.operation context variable is set
as expected during template rendering.
"""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
with local.cwd(src):
build_file_tree(
{
# Ensure the file is regenerated on update.
"copier.yml": "_skip_if_exists: [foo]",
"{{ _copier_conf.answers_file }}.jinja": "{{ _copier_answers|to_yaml }}",
"foo.jinja": "{{ _copier_conf.operation }}",
}
)
git_save(tag="1.0.0")

copier.run_copy(str(src), dst, defaults=True, overwrite=True)
ctx_file = dst / "foo"
assert ctx_file.read_text() == "copy"
# Ensure the file is regenerated on update.
# If we left it, an update would detect custom changes
# that would be reapplied, i.e. an update would leave us with `copy`.
ctx_file.unlink()
with local.cwd(dst):
git_save()
getattr(copier, f"run_{operation}")(str(dst), overwrite=True)
expected = "copy" if operation == "recopy" else operation
assert ctx_file.read_text() == expected
Copy link
Member

Choose a reason for hiding this comment

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

thought: So, let's say that we copy one template and render the value. The file will contain copy.

Now we run it again. The file contains update. The change is OK because the replay renders copy and detects the change.

However, what would happen if we run it yet again? The replay will render copy, but in reality it is update. Now the next one is also update.

Triple-thinking this... are we introducing here a way to make replays non-reproducible and thus maybe making the update algorithm unreliable? Because there's no way Copier could know if last play was a copy or an update.

CC @copier-org/maintainers. Should we just reject this feature to avoid shooting our own feet?

Copy link
Contributor Author

@lkubb lkubb Sep 22, 2024

Choose a reason for hiding this comment

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

Disregarding that _copier_conf.operation is not intended to be used for template content changes, this test for checking the value of operation only works because the file containing {{ _copier_conf.operation }}

  1. is deleted
  2. is marked as skip_if_exists, which means it will be regenerated during update if deleted.

What would happen without it being deleted is the following:

  1. copier copy renders the template, it contains copy
  2. copier recopy overwrites the template, it contains copy
  3. copier update updates the template, it still contains copy. This is valid for each subsequent update.

Why (3):
During _apply_update, old_copy and new_copy are both rendered with _copier_conf.operation == "update". The file contains copy though, which is detected as a custom change and reapplied.

_copier_conf.operation should be documented as a meta variable, intended for templating copier.yml configuration. It cannot be used to influence template contents (reliably).

Edit: I emphasized that operation should only be used in Copier config templates.



def test_exclude_templating_with_operation(tmp_path_factory: pytest.TempPathFactory) -> None:
"""
Ensure it's possible to create one-off boilerplate files
that are not managed during updates.
"""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
exclude = r"{%- if _copier_conf.operation == 'update' %}dumb_boilerplate{%- endif %}"
with local.cwd(src):
build_file_tree(
{
"copier.yml": f"_exclude:\n - \"{exclude}\"",
"{{ _copier_conf.answers_file }}.jinja": "{{ _copier_answers|to_yaml }}",
"dumb_boilerplate": "foo",
"other_file": "foo",
}
)
git_save(tag="1.0.0")
build_file_tree(
{
"dumb_boilerplate": "bar",
"other_file": "bar",
}
)
git_save(tag="2.0.0")
copier.run_copy(str(src), dst, defaults=True, overwrite=True, vcs_ref="1.0.0")
boilerplate = dst / "dumb_boilerplate"
other_file = dst / "other_file"
for file in (boilerplate, other_file):
assert file.exists()
assert file.read_text() == "foo"
with local.cwd(dst):
git_save()
copier.run_update(str(dst), overwrite=True)
assert boilerplate.read_text() == "foo"
assert other_file.read_text() == "bar"
Loading