feat(context): expose a _phase context variable (fix #1883)

[noirbizarre]

This PR exposes a _phase variable which can be one of prompt, tasks, mmigrate and render.
The known phases are grouped into the copier.types.Phase enum.

This implements #1883 and allows fixing copier-org/copier-template-extensions#7

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.93%. Comparing base (6eaa0d7) to head (4cba543).
Report is 132 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1948      +/-   ##
==========================================
- Coverage   98.06%   97.93%   -0.13%     
==========================================
  Files          53       53              
  Lines        5580     5677      +97     
==========================================
+ Hits         5472     5560      +88     
- Misses        108      117       +9     

Flag	Coverage Δ
unittests	97.93% <100.00%> (-0.13%)	⬇️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pawamoy]

Looking super good to me 😄 Thank you for the awesome PRs @noirbizarre!

[noirbizarre]

PR updated with the typo fixes.

You're welcome 🙏🏼 It benefits everybody 👌🏼

[sisp]

Fantastic work again, @noirbizarre! 🙏

Just a minor stylistic request: It seems Python enums should be upper-case¹². I suggest following this guideline.

Footnotes

1. https://mail.python.org/pipermail/python-ideas/2016-September/042340.html ↩

2. https://docs.python.org/3/library/enum.html ↩

[noirbizarre]

Updated to upper case 👍🏼

I, personally, tend to do the opposite with StrEnum and I explicitly chose lower case (and use this convention on all my projects). There is actually no recommendation, the enum PEP itself is all lower case, and StrEnum auto() values are lower cases too. I think having name==value for StrEnum is easier to handle, but I believe it is a personal taste (as highlighted in the thread following Guido's mail)

[sisp]

It seems you're right, there isn't a definitive style rule about enums. 🫣 Thanks for elaborating on this topic (especially the detailed message before you edited it 😉)! Well, now it's upper-case. Shall we leave it like this?

Just a final thought: Shall we expose the phase via the _phase variable (as it is now) or would it be better to rename/move it? E.g.:

• _copier_phase – only _folder_name, now, and make_secret seems to not have the _copier prefix where now and make_secret are both deprecated.
• _copier_conf.phase – or should we only have static values under _copier_conf?

@noirbizarre @copier-org/maintainers Any opinions?

[pawamoy]

I tend to use lowercase too, except when the enum contains names that are reserved keywords like class! No other choice than going uppercase then. As long as we're consistent within Copier's code base, I'm fine with both.

I'm accessing the phase with parent["_copier_conf"]["_phase"] in copier-org/copier-template-extensions#8, it would feel a bit redundant as _copier_phase, but again, consistency is more important IMO.

[sisp]

@pawamoy So, would you tend to be in favor of _copier_phase rather than _phase?

[pawamoy]

Yes, _copier_phase seems good. Do note that we don't strictly need it in _render_context though, see next comment. We also don't need it in render_value, unless some use-cases are shown where a _copier_phase variable whose value is always "prompt" would be used in copier.yml values.

[pawamoy]

Here's my final comprehension of the issue (which may be worth adding as documentation or code comments at least).

• During copier copy, and prompt rendering, context extensions do not get _copier_conf. We can use its absence to decide not to run the context hook.
• During copier update however, and prompt rendering, context extensions do get _copier_conf. We can't use its absence anymore. Instead, we need the phase. It's passed through parent["_copier_conf"]["phase"]: we don't strictly need the additional top-level "_phase" key. We could still keep it to provide more consistency: a _phase / _copier_phase top-level key always present. No strong opinions. I do notice occurrences where parent["_phase"] is unset when testing locally, so if we want this consistency, there are more places this variable must be added.
• Extensions can now also skip running the context hook during tasks and migrations, thanks to the phase.

[noirbizarre]

Oh, I didn't though about all that 😅
Prefixing by _copier makes sense and is consistent with existing ones.
I would go for a root _copier_phase as it is not a configuration, just a runtime state.
I added it in _render_context just to say that it is always available. I don't like to presume future evolution as well as community usage, so I've been on the dumb "it's the current state, and it's always available" approach.

Note that I actually use context extensions during prompt on my projects, and actually this PR can maybe help to have a more general approach on this and assume that extensions can run at any phase. (This might later lead to having copier-template-extensions TemplateExtensionLoader built-in 🤷🏼‍♂️)

Anyway, PR update with _copier_phase naming.

[sisp]

I've thought about this PR some more and also about – what @pawamoy noticed – the phase value being automatically exposed as _copier_conf.phase because most Worker dataclass attributes are automatically included in _copier_conf. As

I would go for a root _copier_phase as it is not a configuration, just a runtime state.

makes sense to me, I think the phase value should not be accessible under _copier_conf though. This PR also reminds me of a similar situation we've had in #1733 where initially a dataclass attribute was used as well and I suggested to use a context variable instead. Now, it feels odd to me to use a dataclass attribute here to manage the phase value because it has no meaning while not rendering, prompting or executing tasks/migrations. How about applying an approach similar to #1733 (review) in this PR as well?

Slightly off-topic: I just realized that #1733 exposes a top-level _operation Jinja variable; we might want to rename it to _copier_operation there for consistency.

[pawamoy]

I agree with all of @sisp's points 🙂

Regarding _copier_conf, the code says "backwards compatibility, remove it?":

copier/copier/main.py

Lines 334 to 339 in 57439e5

	def _render_context(self) -> Mapping[str, Any]:
	"""Produce render context for Jinja."""
	# Backwards compatibility
	# FIXME Remove it?
	conf = asdict(self)
	conf.pop("_cleanup_hooks")

Not sure what it means exactly, but it seems like we wanted to stray away from this anyway.

[sisp]

@pawamoy This comment pre-dates my involvement with Copier, but I don't think the (raw) Worker dataclass attributes are very useful in the Jinja context; useful config values are explicitly added. I'd be in favor of dropping those lines as part of the next major release.

[pawamoy]

Fine by me. I'd suggest making sure _copier_phase is always passed. It's currently not passed from the answers_relpath property, which calls template.render(**self.answers.combined) (template being the answers file name). Looks like it's the only missing location 🙂

[noirbizarre]

PR updated with answers_relpath handled 👍🏼

I saw the deprecation noticed, but it is used by all rendering except those for the prompts.

I'd be in favor of dropping those lines as part of the next major release.

Agreed, but I would go even further (not in this PR to keep it scoped), I would isolate rendering in a single class/module/function/whatever, so future modification of this kind occurs in a single place. It would be easier to maintain and evolve, and easier to test.

[noirbizarre]

Updated: rebased, solved conflicts and adapted to the latest changes.
That was not easy, as 25bc06d introduced a loop:

• Worker.answers_relpath uses Worker.answers.combined as context
• Worker.answers.combined uses Worker.answers.system
• Worker.answers.system is generated by Worker._system_render_context()
• Worker._system_render_context() builds _copier_conf using Worker.answers_relpath

As a consequence, the values used to generate answers_relpath are those from the previous Worker._system_render_context() call.

I found a fix for this PR and added a comment to keep track of it.

[sisp]

There are some merge conflicts because of #1993 now, which mostly reverted the system render context change because of #1977 that also affected this PR.

This PR also reminds me of a similar situation we've had in #1733 where initially a dataclass attribute was used as well and #1733 (review) instead. Now, it feels odd to me to use a dataclass attribute here to manage the phase value because it has no meaning while not rendering, prompting or executing tasks/migrations. How about applying an approach similar to #1733 (review) in this PR as well?

— #1948 (review)

I still think that the Worker.phase attribute isn't the right design. Would you mind trying to use a context variable (#1733 (review)) instead?

[noirbizarre]

Updated:

• conflicts solved
• uses a ContextVar instead of an attribute

[sisp]

Thanks, @noirbizarre!

Sorry for being a little pedantic about the context variable approach. 🫣 But could you remove the default phase value and use a context manager to manage the phase value, such that a particular phase value is only set within an explicitly created context? This way, we reduce the risk of forgetting to set a new phase value and accidentally exposing a previously set one, as ContextVar.get() raises a LookupError when neither a value is set nor a default value is provided.

E.g.:

with use_phase(phase.TASK):
    self._execute_task(self.template.tasks)

(I'm open to better context manager names than use_phase by the way. 😄)

[noirbizarre]

Updated with a context manager.
However, it's not possible to work without a default value or ~2/3 of the tests are failing, which is expected:

• methods are unit tested outside the Worker lifecycle so they are out of any known phase and fail with a LookupError.
• some properties and methods (especially answers_relpath and _print_message) are called outside any phase for computation or user feedback and trigger a rendering, so a lookup
• some operations are done in preparation of a phase outside the phase method itself, example, print statement are outside the methods, as the same method can be used differently depending on the phase (ex: _execute_tasks)

Not having a default value would mean:

• modifying ~2/3 of the tests
• indenting many statements just to use the context manager to ensure the touched property or the method triggering a render/lookup as side effect is not failing

That would mean having most of the codebase modified just to add a variable, and potential conflict with any pending PR, or any PR merged before this one.

So instead, I added a Phase.UNDEFINED as default so tests not relying on it and ran outside the lifecycle don't fail on as an unrelated side effect and the day someone actually hit a case where he needs the _copier_phase in a new phase and setting the phase has been forgotten (that's a lot of conditions), it will have an explicit undefined as value, it will be easy to identify, and it won't make copier failing for him until a fix is released.

WDYT ?

[sisp]

Thanks for explaining why no default value for the context variable won't work. You're right, and I think introducing "undefined" and using it as a default value is a good idea. I also like the implementation of the context manager as a class method of Phase. 👍 Well done! 👏

IIUC, the phase value when rendering the answers file path of external data depends on the phase in which that particular external data is first accessed due to lazy access. Would it be better to override the phase value with "undefined" instead? The meaning of the phase doesn't seem well defined in this case. WDYT?

[sisp]

Very nice, thanks!

IIUC, the phase value when rendering the answers file path of external data depends on the phase in which that particular external data is first accessed due to lazy access. Would it be better to override the phase value with "undefined" instead? The meaning of the phase doesn't seem well defined in this case. WDYT?

WDYT about this?

[noirbizarre]

Make sense 👍🏼
I set it explicitly to undefined and added a comment to explain why.

[sisp]

Almost done, I think. 🤞

[sisp]

Awesome! 🤩 Huge thanks for adding this feature and being incredibly patient with my pedantic review feedback and the many iterations involving code updates and discussions! 🙇 Fantastic work, @noirbizarre! 🏆

Close #1883.