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

Allow configuring Jinja with StrictUndefined #1522

Open
yajo opened this issue Feb 20, 2024 Discussed in #1512 · 7 comments
Open

Allow configuring Jinja with StrictUndefined #1522

yajo opened this issue Feb 20, 2024 Discussed in #1512 · 7 comments
Assignees

Comments

@yajo
Copy link
Member

yajo commented Feb 20, 2024

Discussed in https://github.com/orgs/copier-org/discussions/1512

Originally posted by coretl February 9, 2024
I've been bitten a couple of times by this workflow:

  • Change the name of a variable in copier.yml
  • Forget to rename some of the references to that variable in my template
  • End up with that variable being rendered blank in a text file somewhere when using the template

It would appear that Jinja does this by default, and the way it suggests to raise an error is by supplying undefined=StrictUndefined in its Environment. I tried this in copier.yml:

_envops:
    undefined: StrictUndefined

But it fails because it can't turn the string into a callable:

  File "/venv/lib/python3.11/site-packages/copier/main.py", line 1024, in run_copy
    with Worker(src_path=src_path, dst_path=Path(dst_path), **kwargs) as worker:
  File "/venv/lib/python3.11/site-packages/copier/main.py", line 205, in __exit__
    raise value
  File "/venv/lib/python3.11/site-packages/copier/main.py", line 1025, in run_copy
    worker.run_copy()
  File "/venv/lib/python3.11/site-packages/copier/main.py", line 745, in run_copy
    self._ask()
  File "/venv/lib/python3.11/site-packages/copier/main.py", line 421, in _ask
    jinja_env=self.jinja_env,
              ^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/functools.py", line 1001, in __get__
    val = self.func(instance)
          ^^^^^^^^^^^^^^^^^^^
  File "/venv/lib/python3.11/site-packages/copier/main.py", line 500, in jinja_env
    env = SandboxedEnvironment(
          ^^^^^^^^^^^^^^^^^^^^^
  File "/venv/lib/python3.11/site-packages/jinja2/sandbox.py", line 253, in __init__
    super().__init__(*args, **kwargs)
  File "/venv/lib/python3.11/site-packages/jinja2/environment.py", line 366, in __init__
    _environment_config_check(self)
  File "/venv/lib/python3.11/site-packages/jinja2/environment.py", line 126, in _environment_config_check
    assert issubclass(
           ^^^^^^^^^^^
TypeError: issubclass() arg 1 must be a class

Is there a way to make the copier copy command fail if you use an undefined name in a template?

Related to #313 but with a different aim in mind

@yajo yajo added this to the Soon milestone Feb 20, 2024
@coretl
Copy link
Contributor

coretl commented Mar 4, 2024

Would you like a PR for this?

@yajo
Copy link
Member Author

yajo commented Mar 6, 2024

Of course.

@evalott100
Copy link

I'll start working on this now.

@mschoettle
Copy link

@yajo Would it be acceptable to enable StrictUndefined by default? If so, I'll create a PR.

@yajo
Copy link
Member Author

yajo commented Nov 6, 2024

Makes sense to me, although that'd be a breaking change. WDYT @sisp?

@sisp
Copy link
Member

sisp commented Nov 7, 2024

In general, I think StrictUndefined is a sane default. But I wonder whether we should change the default setting only with a generous deprecation period for template authors to adjust and template users to update their projects. Since Copier's update algorithm relies on replaying the old copy, changing the default setting might break updates for templates that don't handle StrictUndefined correctly yet, e.g. via {% if undefined_var is defined %} instead of {% if undefined_var %}. If we merely added support for StrictUndefined as a non-default setting, then template authors could enable it and replaying the old copy wouldn't be affected. We could deprecate the current (implicit) default setting for a while and then change it, even without a major release in that case.

By the way, #1170 would reduce the trouble of changing the default setting, but the current idea there is flawed. I've been thinking about a different approach, but this needs a proper PoC yet.

@yajo
Copy link
Member Author

yajo commented Nov 20, 2024

Let's start by adding the option, and later the default.

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

No branches or pull requests

5 participants