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

Cleanup template loaders #3555

Merged
merged 10 commits into from Nov 16, 2014

Conversation

Projects
None yet
4 participants
@aaugustin
Member

aaugustin commented Nov 16, 2014

No description provided.

@mjtamlyn

View changes

tests/view_tests/tests/test_debug.py Outdated
with override_with_test_loader({'403.html': 'This is a test template '
'for a 403 Forbidden error.'}):
with override_settings(TEMPLATE_LOADERS=[
('django.templates.loaders.locmem.Loader', {

This comment has been minimized.

@mjtamlyn

mjtamlyn Nov 16, 2014

Member

template (singular)

@mjtamlyn

View changes

tests/view_tests/tests/test_defaults.py Outdated
@@ -40,9 +40,12 @@ def test_custom_templates(self):
Test that 404.html and 500.html templates are picked by their respective
handler.
"""
with override_with_test_loader({
with override_settings(TEMPLATE_LOADERS=[
('django.templates.loaders.locmem.Loader', {

This comment has been minimized.

@mjtamlyn

mjtamlyn Nov 16, 2014

Member

template (singular)

@mjtamlyn

View changes

docs/internals/deprecation.txt Outdated
@@ -82,6 +82,10 @@ details on these changes.
* The backwards compatibility shim to allow ``FormMixin.get_form()`` to be
defined with no default value for its ``form_class`` argument will be removed.
* ``django.template.loader.BaseLoader`` will be removed.

This comment has been minimized.

@mjtamlyn

mjtamlyn Nov 16, 2014

Member

Worth mentioning here that only this import shim is removed, not the whole class.

@mjtamlyn

This comment has been minimized.

Member

mjtamlyn commented Nov 16, 2014

The conceptual and restructuring changes look good. I have not checked every line in detail.

@timgraham

View changes

django/template/loaders/utils.py Outdated
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
from django.utils.deprecation import RemovedInDjango20Warning
from django.utils import lru_cache

This comment has been minimized.

@timgraham

timgraham Nov 16, 2014

Member

I'd order "django.utils import (X)" stuff before any django.utils.(foo)

@timgraham

View changes

django/template/loaders/utils.py Outdated
else:
warnings.warn(
"Function-based template loaders are deprecated. Please use "
"class-based template loaders instead. Inherit base.Loader "

This comment has been minimized.

@timgraham

timgraham Nov 16, 2014

Member

Might help to be explicit here: django.template.loaders.base.Loader

@timgraham

View changes

django/template/loaders/utils.py Outdated
else:
return func
else:
raise ImproperlyConfigured('Loader does not define a "load_template" callable template source loader')

This comment has been minimized.

@timgraham

timgraham Nov 16, 2014

Member

I see you've simply moved this, but I'd cleanup "template source loader" -> "template loader" to use consistent terminology.

This comment has been minimized.

@aaugustin

aaugustin Nov 16, 2014

Member

Actually the message is completely wrong. This exception is raised when TEMPLATE_LOADERS contains something that isn't a string or a tuple/list whose first argument is a string.

warnings.warn(
"django.test.utils.TestTemplateLoader was renamed to "
"django.template.loaders.locmem.Loader.",
RemovedInDjango19Warning, stacklevel=2)

This comment has been minimized.

@timgraham

timgraham Nov 16, 2014

Member

Missing deprecation docs for django.test.utils.TestTemplateLoader.

This comment has been minimized.

@prestontimmons

prestontimmons Nov 16, 2014

Contributor

These changes are very nice. They will definitely simplify some of the patches I've worked on. I'm happy about having a locmem loader for testing.

Will the locmem loader be documented?

Also, I didn't know you could pass args through the TEMPLATE_LOADER setting like this. Is that an official api? The docs don't currently mention this that I'm aware of.

This comment has been minimized.

@aaugustin

aaugustin Nov 16, 2014

Member

@timgraham It's a private API. When I suspect a private API may be used in the wild, I tend to create a deprecation path but not document it.

That doesn't make much sense; I've documented it.

This comment has been minimized.

@aaugustin

aaugustin Nov 16, 2014

Member

@prestontimmons Passing arguments in TEMPLATE_LOADERS is demonstrated in the docs for the cached loader. I don't think it's described as a general purpose mechanism though.

Yes, the locmem loader could be made a public API and yes, the docs could explain how to pass arguments to template loaders in general.

In order to avoid scope creep, I'm not going to add that documentation in this pull request. Could you create tickets about them and maybe submit a pull request?

Thanks!

@timgraham

View changes

docs/releases/1.8.txt Outdated
@@ -1100,3 +1114,7 @@ removed in Django 1.8 (please see the :ref:`deprecation timeline
* ``django.utils.html.fix_ampersands``, the ``fix_ampersands`` template filter,
and ``django.utils.html.clean_html`` are removed.
* Private APIs ``override_template_loaders`` and ``override_with_test_loader``

This comment has been minimized.

@timgraham

timgraham Nov 16, 2014

Member

Hm, I've realized there's now a slight ambiguity where this should be listed. Currently this list contains only those features which went through the normal deprecation cycle. I think we should probably keep it that way and move this to backwards incompatible changes.

This comment has been minimized.

@aaugustin

aaugustin Nov 16, 2014

Member

I moved them to the backwards-incompatible changes section. Release notes are very hard to navigate :-/

aaugustin added some commits Nov 15, 2014

Fixed regression in 4dc4d12. Refs #21598.
That commit contained a mistake that resulted in the use_cached_loader
option of override_with_test_loader being ignored. As a consequence some
configurations weren't exercised any more by the test suite.
Moved all template loaders under django.template.loaders.
Reformatted the code of base.Loader according to modern standards.

Turned the test template loader into a regular locmem.Loader -- but
didn't document it.

Added a normal deprecation path for BaseLoader which is a public API.

Added an accelerated deprecation path for TestTemplateLoader which is
a private API.
Removed the "test:" prefix from locmem template identifiers.
Since it isn't branded as a test utility any more and could be used for
other purposes than test code, that prefix no longer makes sense.

It wasn't used anywhere either.
Removed skip_template argument of locmem.Loader.load_template_source.
It didn't do anything, wasn't documented and wasn't used anywhere.
Refactored listing template subdirectories in apps.
This change has the nice side effect of removing code that ran at import
time and depended on the app registry at module level -- a notorious
cause of AppRegistryNotReady exceptions.
Removed override_template_loaders and override_with_test_loader.
They can be replaced with override_settings and that makes the
corresponding tests much more obvious.
Removed obsolete comment.
It didn't account for class-based template loaders.
Refactored getting the list of template loaders.
This provides the opportunity to move utility functions specific to the
Django Template Language outside of django.template.loader.
Used get_template_loaders in the cached loader.
This ensures that enabling the cached loader doesn't change behavior.

(Before this commit, it did when the list contained unusable loaders.)

@aaugustin aaugustin force-pushed the aaugustin:cleanup-template-loaders branch to e87bee6 Nov 16, 2014

@aaugustin aaugustin merged commit e87bee6 into django:master Nov 16, 2014

1 check was pending

default Merged build started.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment