Fixed #27359 -- Allowed multiple DTL backends with Engine.get_default(). #8000
Conversation
|
Yes this approach seems reasonable to me. |
|
@aaugustin OK. Thanks for the review. I'll adjust the docs to match and then ping you for final OK. |
timgraham
changed the title
Engine.get_default()
|
I've adjusted the docs. Should be good for review. Aside: |
tests/template_tests/test_engine.py
Outdated
| without passing an explicit engine. | ||
| """ | ||
| engine = Engine.get_default() | ||
| self.assertTrue(isinstance(engine, Engine)) |
There was a problem hiding this comment.
As you've documented that the first engine will be returned, the test should probably check that, not just that any engine gets returned. (Maybe the name can be asserted?)
There was a problem hiding this comment.
@das-g Not sure. All we're testing there is that list is ordered, but we know that...
The assertTrue here isn't really testing very much. The test is really that the get_default call doesn't raise an error.
Bottom line is, I don't really mind. (See below)
There was a problem hiding this comment.
Hmm.
It seems the name isn't available:
(Pdb) pp engine.__dict__
{'app_dirs': False,
'autoescape': True,
'builtins': ['django.template.defaulttags',
'django.template.defaultfilters',
'django.template.loader_tags'],
'context_processors': [],
'debug': False,
'dirs': [],
'file_charset': 'utf-8',
'libraries': {'admin_list': 'django.contrib.admin.templatetags.admin_list',
'admin_modify': 'django.contrib.admin.templatetags.admin_modify',
'admin_static': 'django.contrib.admin.templatetags.admin_static',
'admin_urls': 'django.contrib.admin.templatetags.admin_urls',
'bad_tag': 'template_tests.templatetags.bad_tag',
'cache': 'django.templatetags.cache',
'custom': 'template_tests.templatetags.custom',
'i18n': 'django.templatetags.i18n',
'inclusion': 'template_tests.templatetags.inclusion',
'l10n': 'django.templatetags.l10n',
'log': 'django.contrib.admin.templatetags.log',
'static': 'django.templatetags.static',
'staticfiles': 'django.contrib.staticfiles.templatetags.staticfiles',
'subpackage.echo': 'template_tests.templatetags.subpackage.echo',
'tag_27584': 'template_tests.templatetags.tag_27584',
'testtags': 'template_tests.templatetags.testtags',
'tz': 'django.templatetags.tz'},
'loaders': [('django.template.loaders.cached.Loader',
['django.template.loaders.filesystem.Loader'])],
'string_if_invalid': '',
'template_builtins': [<django.template.library.Library object at 0x10d5a19b0>,
<django.template.library.Library object at 0x10d98acc0>,
<django.template.library.Library object at 0x10d5b64a8>],
'template_libraries': {'admin_list': <django.template.library.Library object at 0x10e5107f0>,
'admin_modify': <django.template.library.Library object at 0x10e510f28>,
'admin_static': <django.template.library.Library object at 0x10e510f98>,
'admin_urls': <django.template.library.Library object at 0x10df33518>,
'bad_tag': <django.template.library.Library object at 0x10e525438>,
'cache': <django.template.library.Library object at 0x10e4ef748>,
'custom': <django.template.library.Library object at 0x10e525e10>,
'i18n': <django.template.library.Library object at 0x10e4effd0>,
'inclusion': <django.template.library.Library object at 0x10e534828>,
'l10n': <django.template.library.Library object at 0x10e510208>,
'log': <django.template.library.Library object at 0x10e5250b8>,
'static': <django.template.library.Library object at 0x10d416d30>,
'staticfiles': <django.template.library.Library object at 0x10e525208>,
'subpackage.echo': <django.template.library.Library object at 0x10e53e9e8>,
'tag_27584': <django.template.library.Library object at 0x10e53e908>,
'testtags': <django.template.library.Library object at 0x10e53ed30>,
'tz': <django.template.library.Library object at 0x10e5105f8>}}
... because get_default() returns the unwrapped Engine instance, and not the backend.
There was a problem hiding this comment.
The only way to check the identity of the engine is to re-perform the exact filtering logic from get_default, which—being the method under test—wouldn't add anything. (Except copy/paste maintenance overhead.)
Prior to this PR, there wasn't explicit coverage on this method. Both branches are now tested.
The first result is just the behaviour from list—which presumably we can rely on.
On the principle of "Obviously no errors, or no errors that are obvious", I'd say the implementation is simple enough not to need more checking.
We can add the extra check if it's insisted upon, but I think it better without.
|
@timgraham (Being the Django Fellow) Sorry for the noise but do we need to do anything to push this forward, or will it resolve in time, or...? (Not sure of next steps.) Thanks. |
https://code.djangoproject.com/ticket/27359#ticket-next-steps says
|
|
I've now taken the time to go through the patch review checklist:
✅ Yes, it does.
✅ Yes, as far as I can tell.
I didn't find any, nor were any found by
✅ Yes, there are two new tests (one for existing unchanged behavior and one for the new behavior) and one of them does fail when applied without the changes to the production code. Though, I'm not sure this was a bug, as the old observed behavior corresponded to the old expected (i.e. documented) behavior. Thus let's also look at
Well, this is a changed feature, not a new one. But at least some of these points should probably apply for changed features, too.
✅ Yes, they do. ❓ Though, the standard case of exactly one template engine being configured isn't covered explicitly. Looking at the current implementation of
❌ No, there isn't.
❌ There isn't, and a
(Doesn't apply.)
✅ Conforms, as far as I can tell, and
❓ While this change is strictly speaking backwards incompatible, I can't imagine someone relying on the old behavior in any situation where it differs from the new behavior.
✅ Yes, it is.
❌ It's multiple commits, not one. (When squashing them, please also make the commit message match the requested format.)
❌ I can't find you in the |
|
@das-g Thanks for the review. I shall adjust accordingly. |
|
Also, I have submitted the CLA. |
|
OK. I've updated the PR with everything except this one:
Not sure what to do about that. Now just to squash and adjust the commit message. |
|
OK. All squashed. I think it should be ready to go. ...? |
Well, that was about
of the patch review checklist and you did add a note to |
|
@das-g Awesome. Thanks. (My first Django-patch! — I have to get it over the line... 😣) |
There was a problem hiding this comment.
I've looked at the patch again and only have minor nitpicks.
@das-g Awesome. Thanks. (My first Django-patch! — I have to get it over the line... 😣)
Mine, too (I'm not affiliated with Django, other than using it, too), so don't take my nitpicking too serious. 😉 Also, thank you very much for the effort you've put in this. I, too, would really like this change to be integrated. 😃
django/template/engine.py
Outdated
| @@ -58,7 +58,8 @@ def get_default(): | |||
| """ | |||
| When only one DjangoTemplates backend is configured, return it. | |||
|
|
|||
| Raise ImproperlyConfigured otherwise. | |||
| If multiple DjangoTemplates backends are configured, returns the first | |||
There was a problem hiding this comment.
Phrase that imperatively ('return' instead of 'returns'), analogous to the change you supersede here.
docs/ref/templates/api.txt
Outdated
|
|
||
| Previously would raise | ||
| :exc:`~django.core.exceptions.ImproperlyConfigured` if multiple engines | ||
| were configured. |
There was a problem hiding this comment.
Analogous to the examples in the "Documenting new features" documentation, I'd describe the new behavior rather than the old one, e.g.
.. versionchanged:: 2.0
Permitted multiple engines to be configured, so that
:exc:`~django.core.exceptions.ImproperlyConfigured` isn't raised anymore
if multiple engines are configured.or even just
.. versionchanged:: 2.0
Permitted multiple engines to be configured.or
.. versionchanged:: 2.0
Permitted multiple engines to be configured, returning the first one if there is more than one.
tests/template_tests/test_engine.py
Outdated
| @@ -21,6 +22,45 @@ def test_basic_context(self): | |||
| ) | |||
|
|
|||
|
|
|||
| class GetDefaultTest(SimpleTestCase): | |||
|
|
|||
| def test_single_dtl_engine_configured(self): | |||
There was a problem hiding this comment.
I'd put this test after the next one, so you get the very logical order 0 engines, 1 engine, 2 engines. Also, you might want to explicitly set the TEMPLATES setting for this test, too
- as to not rely on the number of engines in the global setting (although that is unlikely to change)
- (more importantly) to communicate to the reader what scenario is being tested in comparison to the other two tests
docs/ref/templates/api.txt
Outdated
| Returns the the underlying :class:`Engine` from the first configured | ||
| :class:`~django.template.backends.django.DjangoTemplates` engine. If no | ||
| engines are configured will raise | ||
| :exc:`~django.core.exceptions.ImproperlyConfigured`. |
There was a problem hiding this comment.
👍 good catch by the way.
(That change was missing in the original patch, wasn't it?)
|
@das-g Thanks again for the input. I agree with your suggestions. I'll sort them out later. |
|
Adjusted for feedback. |
docs/ref/templates/api.txt
Outdated
| :class:`~django.template.backends.django.DjangoTemplates` engine, this | ||
| method returns the underlying :class:`Engine`. In other circumstances it | ||
| will raise :exc:`~django.core.exceptions.ImproperlyConfigured`. | ||
| Returns the the underlying :class:`Engine` from the first configured |
There was a problem hiding this comment.
Oops, noticed this only just now: There's a repeated "the" here.
There was a problem hiding this comment.
Good catch. Updated.
|
As advised, after having gone through the patch review checklist again, I've marked #27359 as "Ready for checkin" and it now says:
@timgraham @aaronelliotross Can this be merged as-is or does it need more work? |
|
Looks good. I merged the non-regression tests in 7019724 and pushed a few edits to this branch. Care to review those edits? |
|
@timgraham I too find have found the squashing to be a inconvenient and a bit premature — it's much easier to review when the changes are separate. Do you think there's room to move to a process whereby we — this is my first PR for Django so that's a generous "we" 🙂 — use GitHub's squash and merge, rather than squashing beforehand? (Perhaps the proposed commit message could be posted as a comment.) |
|
Also, your git-foo is very high. I'm not quite sure how you managed to pull just the tests out of the squashed commit whilst maintaining the author info. 👏 |
django/template/engine.py
Outdated
| if isinstance(engine, DjangoTemplates) | ||
| ).engine | ||
| except StopIteration: | ||
| raise ImproperlyConfigured('No DjangoTemplates backend is configured.') |
There was a problem hiding this comment.
I'm not sure this next() dance is really required, I find the following much easier to read
for engine in engines.all():
if isinstance(engine, DjangoTemplates):
return engine.engine
raise ImproperlyConfigured('No DjangoTemplates backend is configured.')There was a problem hiding this comment.
My version had this:
django_engines = [engine for engine in engines.all()
if isinstance(engine, DjangoTemplates)]
if len(django_engines) == 0:
raise ImproperlyConfigured(
"No DjangoTemplates backend is configured.")
else:
# Unwrap the first Engine instance inside DjangoTemplates
return django_engines[0].engine...which was closer to the original.
There was a problem hiding this comment.
Yes, your version is simpler, Simon.
…lates engine if multiple are defined.
|
This patch is quite small so I don't find it difficult to review squashed changes. For large commits, a follow up commit may be preferred so someone doesn't have to review the entire patch again. To set the author of a git commit: |
Fixes #27359
Adds a failing test case for the issue and a naive fix.
Users should employcollections.OrderedDictif they need to control this prior to Python 3.6.Alternative proposals:
OPTIONto the config dict.This is something of an edge case, and so, (IMO) 'pick the first' should be sufficient.