Skip to content

Fail fast on invalid Jinja2Template instantiation parameters#2568

Merged
Kludex merged 2 commits intoKludex:masterfrom
pdelagrave:fix_templates_env_dir
Apr 20, 2024
Merged

Fail fast on invalid Jinja2Template instantiation parameters#2568
Kludex merged 2 commits intoKludex:masterfrom
pdelagrave:fix_templates_env_dir

Conversation

@pdelagrave
Copy link

@pdelagrave pdelagrave commented Apr 12, 2024

Summary

Calling Jinja2Template() with both directory and env shouldn't be allowed. When both parameters were used, the passed env was silently ignored in favor of creating a new one with the provided directory and the deprecated env_options.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Calling `Jinja2Template()` with both `directory` and `env` shouldn't be allowed. When both parameters were used, the passed `env` was silently ignored in favor of creating a new one with the provided `directory` and the deprecated `env_options`.
@pdelagrave pdelagrave force-pushed the fix_templates_env_dir branch from 402545d to 05c1d10 Compare April 12, 2024 13:26
Copy link
Owner

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

This reflects the overrides. 👍

I think we should probably remove this assertion at some point... The type checker is able to catch this anyway.

@Kludex Kludex enabled auto-merge (squash) April 20, 2024 07:53
@Kludex Kludex disabled auto-merge April 20, 2024 07:53
@Kludex Kludex enabled auto-merge (squash) April 20, 2024 07:53
@Kludex Kludex merged commit 9cf26ee into Kludex:master Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants