Adding the template loader to returned Origins

[jdotjdot]

This is for compatibility with Django 1.9 + Django Compressor.

I found that running python manage.py compress was throwing an error:

Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/core/management/__init__.py", line 353, in execute_from_command_line
    utility.execute()
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/core/management/__init__.py", line 345, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/core/management/base.py", line 348, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/core/management/base.py", line 399, in execute
    output = self.handle(*args, **options)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/compressor/management/commands/compress.py", line 286, in handle
    self.compress(sys.stdout, **options)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/compressor/management/commands/compress.py", line 181, in compress
    nodes = list(parser.walk_nodes(template))
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/compressor/offline/django.py", line 150, in walk_nodes
    for node in self.walk_nodes(node, original):
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/compressor/offline/django.py", line 146, in walk_nodes
    for node in self.get_nodelist(node, original):
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/compressor/offline/django.py", line 128, in get_nodelist
    return handle_extendsnode(node, block_context=None, original=original)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/compressor/offline/django.py", line 34, in handle_extendsnode
    compiled_parent = extendsnode.get_parent(context)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/template/loader_tags.py", line 148, in get_parent
    return self.find_template(parent, context)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/template/loader_tags.py", line 128, in find_template
    template_name, skip=history,
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/template/engine.py", line 157, in find_template
    name, template_dirs=dirs, skip=skip,
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/template/loaders/cached.py", line 60, in get_template
    template_name, template_dirs, skip,
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/template/loaders/base.py", line 40, in get_template
    contents = self.get_contents(origin)
  File "/Users/Me/.virtualenvs/MyProject/lib/python2.7/site-packages/django/template/loaders/cached.py", line 28, in get_contents
    return origin.loader.get_contents(origin)
AttributeError: 'NoneType' object has no attribute 'get_contents'

The issue was due to the fact that Django compressor calls origin.loader.get_contents(...), but the Origin instances being returned by the apptemplates loader didn't bind the loader to the Origin. This fixes that.

All of the tests passed. I am not sure if Django Compressor is the only use case for this, but it wouldn't surprise me if there were others, since Django's loaders now seem to bind the loader to the Origin.

Update: The only tests that fails is a pylint one because one of the arguments is unused in the <Django1.9 compatibility version of the function definition. I think this is how it should be but don't know how to silence pylint here. Or, if you'd prefer I find a way to somehow use the extra argument, let me know what your stylistic preference is.

[bittner]

@jdotjdot Thanks for finding out and opening this PR!

Cover All Parameters, Maybe?

Shouldn't we maybe also initialize also parameter template_name in Origin's constructor? Fixing the None value for one parameter and ignoring the same issue for another one feels like begging for another bugfix. Though I must admit I don't know about any consequences of this (if there are any outside of the Origin class).

Silence PyLint Warning

You can add the following at the end of the criticized line 47 to silence the PyLint warning:

# pylint: disable=unused-argument

I think this is safe to do in this one-line function. We won't miss any potential problems.

Add a Test?

Can we add a test for this very problem? The current tests also passed before, so we'd need a new one to cover the problem domain. It should fail without your changes. Can you think of one?

[jdotjdot]

I added the template_name to Origin and the pylint line. The test I'm a little uncertain of, though, and the reason is that I'm not sure that providing the loader is actually required. As you said, I don't know what the consequences would be, and django-apptemplates did work fine in general without it—it was only with django-compressor specifically that I ran into trouble.

[bittner]

The test I'm a little uncertain of, though, and the reason is that I'm not sure that providing the loader is actually required. As you said, I don't know what the consequences would be, and django-apptemplates did work fine in general without it

Maybe I was not clear enough:

• Adding the test would make things more certain with regards to django-compressor.

Think of that: Your fix has been tested with django-compressor

1. only on a Mac
2. only with Python 2.7
3. only with a specific version of Django (the one you have installed currently)
4. only with a specific version of django-compressor (the one you have installed currently)
5. only a single time, manually (it will never be repeated in the future to avoid regression)

The tests we have in this repository, on the other hand, all passed even before you added your changes! So, what we'd need to do really is:

1. Add a test involving django-compressor that makes our tests fail (obviously only with Django 1.9+) without your changes
2. Add your changes and show that the same test passes

(As for the "consequences" I intended the additional change of setting also the template_name parameter.)

[bronger]

I don't use django-compressor but observe this when using django-redis. This PR in its current form fixes it.

[bittner]

@bronger I appreciate your observations. They are valuable input.

@jdotjdot Do you want to address the comments I have made on your PR?

For the tests it's important to prove that the current situation makes them fail, and the changes of this PR make them pass. Then we have certainty. Ideally, we prove that this PR fixes compatibility with both django-redis and django-compressor.

[bittner]

Hi @jdotjdot,

is this PR still relevant?

Would you be interested in taking this forward, the final steps?

[bittner]

Fixed via 21c15824a08fd28ea588b4a32b2ca724de0be198. Thanks @jdotjdot!

[jdotjdot]

Thanks for merging!

[bittner]

Sorry it took so long! Well, we have no tests for the issue now, really. Let's cross our fingers.

I had to redo the changes due to a conflict after merging the other PR, that's why your name isn't in the commits, I apologize, but I put it into the README. Hope that's fine.

[jdotjdot]

It's totally fine; I don't care at all.

On Sun, Jul 9, 2017 at 5:38 PM, Peter Bittner notifications@github.com wrote:

Sorry it took so long! Well, we have no tests for the issue now, really. Let's cross our fingers.

I had to redo the changes due to a conflict after merging the other PR, that's why your name isn't in the commits, I apologize, but I put it into the README. Hope that's fine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bittner/django-apptemplates/pull/1#issuecomment-313965491, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRHQRjBPVkGL7IHfoeW99W0nTSkSATwks5sMUhQgaJpZM4Iqzin .