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

Make from_string() and get_template() return backend-specific template #2400

Merged
merged 8 commits into from
Oct 12, 2024

Conversation

mthuurne
Copy link
Contributor

@mthuurne mthuurne commented Oct 9, 2024

Each template backend will only return instances of its own specific template class. For example, django.template.backends.jinja2.Jinja2 only returns django.template.backends.jinja2.Template instances.

Each backend-specific template class has some unique features that a Django app or tool might be interested in.

I had to include a bunch of cleanups as well, because the original signatures of _EngineTemplate.render() and django.template.backends.dummy.Template.render() were incompatible.

The implementation of `make_context()` explicitly rejects everything
other than `dict` or `None` by raising `TypeError`.
[DEP 182](https://github.com/django/deps/blob/main/final/0182-multiple-template-engines.rst#backends-api)
states: "If `context` is provided, it must be a `dict`.".

While the `Jinja2` and `TemplateStrings` backends support arbitrary
mappings, `DjangoTemplates` calls `make_context()`, which only
supports `dict` (and `None`).
Only the Django Template Language backend outputs `SafeString`.
All of the argument types are Django types, so there is no need to
avoid annotating them. Only the Jinja2 native template type is
unknown to us.
The implementation of the `render()` method uses the standard library
method `string.Template.safe_substitute()` to do the actual formatting,
which accepts other value types than `str` and simply calls `__str__()`
on them.

The `safe_substitute()` method is annotated in typeshed with `object`
as the value type for the mapping. However, as `_EngineTemplate` uses
`Any` instead, I went with that.
@mthuurne
Copy link
Contributor Author

mthuurne commented Oct 9, 2024

This code:

class Template(string.Template, _EngineTemplate):

triggers this mypy error on Python 3.8:

django-stubs/template/backends/dummy.pyi:13: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]

What would be the best solution?

  • drop commit 509e368 that adds _EngineTemplate as a base class (since it's a protocol, we don't need to inherit from it)
  • wait for django-stubs to drop Python 3.8 support (since it's EOL now)

For now, I'll revert the commit, to make sure that all the CI checks pass before further review.

… protocol"

This reverts commit 509e368.

Inheriting from both `string.Template` and `typing.Protocol` causes
mypy to report a metaclass conflict on Python 3.8.
@@ -23,6 +21,6 @@ class BaseEngine:
class _EngineTemplate(Protocol):
def render(
self,
context: Context | dict[str, Any] | None = ...,
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove this? Context can be passed to render()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the commit comment of 2a070e0: not all implementations of this render() method accept Context and the spec states that dict must be used.

The naming is a bit confusing, as there are multiple classes named Template and multiple render() methods with different interfaces. The implementer of _EngineTemplate is django.template.backends.django.Template and the render() method of that class doesn't accept Context, as it calls make_context() which explicitly rejects anything other than None and dict.

The django.template.base.Template.render() method does accept Context and in fact requires it, but that is not an implementation of the _EngineTemplate protocol, despite using the same class and method name.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I got it! Thanks!

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@sobolevn sobolevn merged commit 1d3b071 into typeddjango:master Oct 12, 2024
38 checks passed
@mthuurne mthuurne deleted the backend-specific-template branch October 13, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants