feat: Add _operation variable

[lkubb]

• Makes exclude configuration templatable.
• Adds an _operation variable to the rendering contexts for exclude and tasks, representing the current operation - either copy, recopy or update.
• Adds the (semi-)undocumented _copier_python, _copier_conf.os and _folder_name variables to the docs in a second commit (since I restructured them anyways) extracted into docs: Restructure context vars, add undocumented ones #1856, will rebase once it's merged rebased

Notes:
This was proposed here: #1718 (comment)

I hope the way it's implemented and tested is as intended by @yajo. The tests are quite synthetic, I would expect the usual application to be _operation (!/=)= "update".

Fixes: #1725
Fixes: #1625

Alternative to #1732

[yajo]

The docs would need to reflect this change too.

[yajo]

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

[lkubb]

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.

[yajo]

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?

[lkubb]

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.

[codecov]

Codecov Report

Attention: Patch coverage is 98.61111% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.75%. Comparing base (2e7629e) to head (2a80d01).

Files with missing lines	Patch %	Lines
copier/types.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1733      +/-   ##
==========================================
- Coverage   97.67%   95.75%   -1.92%     
==========================================
  Files          49       50       +1     
  Lines        5238     5306      +68     
==========================================
- Hits         5116     5081      -35     
- Misses        122      225     +103     

Flag	Coverage Δ
unittests	95.75% <98.61%> (-1.92%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lkubb]

@yajo Is there anything I still need to do here? Just making sure you noticed the changes. :)

[yajo]

Sorry for taking so long to review! I don't have a lot of time and this one required deep thinking.

[yajo]

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?

[sisp]

Sorry for taking so long to give feedback. 🫣 This PR really needs some deep thinking.

Adding the attribute operation to the Worker class seems conceptually wrong to me because we'd make the operation mode part of its state while it also has methods to execute either operation (run_copy() and run_update()). Thus, it would be possible to set the operation attribute to "update" and call run_copy() (or set the operation attribute to "copy" and call run_update()) which would lead to incorrect behavior. This indicates a bad design to me.

How about using a context variable to manage the operation context? This is a simplified example of what I mean:

from __future__ import annotations

import sys
from contextvars import ContextVar
from dataclasses import dataclass, replace
from functools import wraps
from typing import Callable, Literal, TypeVar

if sys.version_info >= (3, 10):
    from typing import ParamSpec
else:
    from typing_extensions import ParamSpec


Operation = Literal["copy", "update"]
P = ParamSpec("P")
R = TypeVar("R")

_operation: ContextVar[Operation] = ContextVar("_operation")


def as_operation(value: Operation) -> Callable[[Callable[P, R]], Callable[P, R]]:
    def _decorator(func: Callable[P, R]) -> Callable[P, R]:
        @wraps(func)
        def _wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
            token = _operation.set(_operation.get(value))
            try:
                return func(*args, **kwargs)
            finally:
                _operation.reset(token)

        return _wrapper

    return _decorator


@dataclass
class Worker:
    template: str

    @as_operation("copy")
    def run_copy(self) -> None:
        print(f"Worker({self.template}).run_copy(): {_operation.get()}")

    @as_operation("update")
    def run_update(self) -> None:
        print(f"Worker({self.template}).run_update(): {_operation.get()}")
        replace(self, template="old").run_copy()
        replace(self, template="new").run_copy()


worker = Worker("new")

print("$ copier copy ...")
worker.run_copy()
# -> Worker(new).run_copy(): copy

print("$ copier update ...")
worker.run_update()
# -> Worker(new).run_update(): update
# -> Worker(old).run_copy(): update
# -> Worker(new).run_copy(): update

The Worker.run_copy() and Worker.run_update() methods both set their operation context via the decorator as_operation(). The decorator wraps the decorated method and sets the operation context to the provided value only when the operation context has not been set yet; if the operation context has been set, it remains as is. Thus, a call of Worker.run_update() sets the operation context to "update", and when Worker.run_copy() gets called within this context, the operation context will remain "update".

Using ContextVar is thread-safe. Also, the following requirement stated in the Python docs

Important: Context Variables should be created at the top module level and never in closures. Context objects hold strong references to context variables which prevents context variables from being properly garbage collected.

is met.

I agree that the Jinja variable _copier_conf.operation only seems to make sense when rendering template expressions in copier.yaml. And although the update seems to produce correct results, the fact that the _copier_conf.operation value is unstable during an update (while rendering files etc.), it is asking for trouble IMO. So, how about omitting the _copier_conf.operation variable in the Jinja context while rendering files, directories, and path names? We'd need to document this behavior of course.

WDYT, @lkubb @yajo?

[lkubb]

Sorry for taking so long to give feedback. 🫣 This PR really needs some deep thinking.

@sisp Thank you for your thoughtful feedback and sorry for taking my time to reply as well.

Adding the attribute operation to the Worker class seems conceptually wrong to me because we'd make the operation mode part of its state while it also has methods to execute either operation (run_copy() and run_update()).

Agreed, I wasn't really happy with it myself, also because it meant the forced inclusion of the operation in inappropriate contexts. Part of this design was caused by the tight coupling of the Copier worker with the renderer imho, I wasn't sure how to solve it properly without a major refactoring.

Thus, it would be possible to set the operation attribute to "update" and call run_copy() (or set the operation attribute to "copy" and call run_update()) which would lead to incorrect behavior. This indicates a bad design to me.

Note that it's only the former situation that leads to incorrect behavior, the latter is the expected path since the run_update method replaces the operation value itself.

How about using a context variable to manage the operation context? This is a simplified example of what I mean:

Thanks a lot for your well thought-out proposal. I agree this design is superior and am currently implementing it. I'm still contemplating a detail since it aggravates a flaw I just noticed in the exclude rendering logic by decoupling the context switch implementation from the worker:

Since match_excludes is a cached property whose value now depends on the _operation, a changed _operation is not reflected in its value if an instance has already been used in a different context. This is not an issue in practice currently since a context switch always coincides with the creation of a new worker instance via replace, but it's definitely a gotcha in case this changes at some point.

[Note: An analog of this is currently present in match_skip, especially if externally mutated instance attributes need to be considered in worker methods].

I might need to address this by deleting the property's cached value explicitly in run_(re)copy/run_update to force regeneration, which is still a bit smelly, or turning it into a cached function instead (less smelly, but a breaking change).

So, how about omitting the _copier_conf.operation variable in the Jinja context while rendering files, directories, and path names? We'd need to document this behavior of course.

👍 Your proposal allows to restrict it to the scopes where it makes sense, namely exclude and task templating.

[sisp]

Thanks for updating this PR, @lkubb! 👍 This looks very good, just a couple of quite minor requests and ideas from my side.

[mschoettle]

Adds an _operation variable to the rendering contexts for exclude and tasks, representing the current operation - either copy, recopy or update.

What this PR adds is exactly what I am missing.

I recently started using copier for an infrastructure project using compose, env files etc. with various different configuration options.

There are some things, such as questions/answers and tasks, that I don’t want to ask/run on update. And, other tasks should only run when an update is performed.

I am wondering what the rationale is behind not making the operation available to questions (conditions), if I read the above correctly. Would it be possible to use it in when?

[lkubb]

@mschoettle

There are some things, such as questions/answers and tasks, that I don’t want to ask/run on update. And, other tasks should only run when an update is performed.

For the latter I'd recommend using migrations. They can be configured to run independent of the involved versions.

I am wondering what the rationale is behind not making the operation available to questions (conditions), if I read the above correctly. Would it be possible to use it in when?

No, the reason is that when == False variables are not saved, so you'd lose previously provided answers during an update. If I understood your requirements correctly, there's a workaround described in #1574 (comment)

[sisp]

Great! Just some absolutely minor remarks. Once #1856 has been merged and this one rebased, I think we can merge. But I'd like to give also @yajo the opportunity to give feedback, as he performed the first review iteration.

[sisp]

Fantastic work, @lkubb! 👌 LGTM! 🎉