Opened 16 years ago

Closed 14 years ago

Last modified 12 years ago

#6587 closed (fixed)

remove django.templates __path__ hacking

Reported by: oyvind Owned by: Andrew Badr
Component: Template system Version: dev
Severity: Keywords: path hacking remove
Cc: cgrady@…, mathieu.damours@…, sdeasey@…, andrewbadr.etc@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

As mentioned in VersionOneFeatures

I made a attempt at fixing it, posting git patch

Attachments (16)

new_template_lib_loader.diff (2.8 KB ) - added by oyvind 16 years ago.
new template library loader
new_template_lib_loader_2.diff (3.3 KB ) - added by oyvind 16 years ago.
better error messages, no conflicting library names, better naming of variables
new_template_lib_loader_3.diff (3.4 KB ) - added by oyvind 16 years ago.
Do not walk subdirs in os.walk
new_template_lib_loader_4.diff (4.5 KB ) - added by oyvind 16 years ago.
get_library no longer uses django.templatetags.module
new_template_lib_loader_5.diff (5.1 KB ) - added by oyvind 16 years ago.
More use of import, tests pass
new_template_lib_loader_6.diff (4.5 KB ) - added by oyvind 16 years ago.
populate app_label in templatetags/init..py, import_library is own function
new_template_lib_loader_7.diff (5.0 KB ) - added by oyvind 16 years ago.
better naming of variables, some code fixes, added some docstrings
new_template_lib_loader_8.diff (4.6 KB ) - added by oyvind 16 years ago.
All tests pass
new_template_lib_loader_9.diff (4.5 KB ) - added by oyvind 16 years ago.
removed os import
new_template_lib_loader_10.diff (70.0 KB ) - added by oyvind 16 years ago.
fixed import to get the correct module, moved defaulttags and defaultfilters to avoid circular imports, fixes #6579
new_template_lib_loader_11.diff (134.6 KB ) - added by oyvind 16 years ago.
forgot to remove some files in the commit, again
new_template_lib_loader_12.diff (135.7 KB ) - added by oyvind 16 years ago.
new loader using imp, may need some cleaup, and decide if add_to_builtins should change, and if a import should use a list instead of the getattr solution
new_template_lib_loader_13.diff (135.6 KB ) - added by oyvind 16 years ago.
removed import using getattr
new_template_lib_loader_14.diff (135.0 KB ) - added by oyvind 16 years ago.
Final patch, i hope :)
template_libs_t6587_r12288_v1.diff (145.9 KB ) - added by Andrew Badr 14 years ago.
t6587-r12291.1.diff (4.8 KB ) - added by Russell Keith-Magee 14 years ago.
RC1 of template load refactoring

Download all attachments as: .zip

Change History (49)

by oyvind, 16 years ago

new template library loader

by oyvind, 16 years ago

better error messages, no conflicting library names, better naming of variables

by oyvind, 16 years ago

Do not walk subdirs in os.walk

by oyvind, 16 years ago

get_library no longer uses django.templatetags.module

comment:1 by oyvind, 16 years ago

all tests in runtests.py templates now pass

comment:2 by Simon Greenhill <dev@…>, 16 years ago

Triage Stage: UnreviewedReady for checkin

comment:3 by Malcolm Tredinnick, 16 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

A good start. Needs a little more tweaking, though.

Line 19 in the new version of templatetags/__init__.py can't be right. It looks like you're trying to replicate Python's import behaviour, but that's not the way to do it. I would be cautious of anything trying to do with this work without using at least some functions from Python's imp module. See below (the "Finally,..." paragraph) for more here: I feel a bit suspicious about the whole approach in this part of the code.

Lines 8 and 9 in the same file cry out for a better solution. One possible, probably for another ticket, is to add a "tags to load always" list in settings. This would help people who are poking into the builtin tags as well. We could add django.templatetags.* to that list.

Even simpler, and probably sufficient for the purposes of this ticket is to iterate over ['django'] + list(settings.INSTALLED_APPS). That avoids having some special list we have to remember to update if we add more tags in there.

Finally, the approach in get_library_names() looks complex. You're doing a lot of computation there in the hope of getting one hit on the returned dictionary when you try to load a tag. Is it possible to let Python do more of the work here. Loop over the list of directories (<app_name>+'.templatetags') and try appending what is being loaded to the end of that and importing. As soon as you don't get an import error, stop with success. That will also mean if you try to load the same template tags in multiple templates in the the same process, Python will just reuse the one already in memory, instead of walking all those directories again. Push less of the work down into get_library_names() and try to use Python's builtin import facilities (esp. __import__() a bit more.

comment:4 by Collin Grady <cgrady@…>, 16 years ago

Cc: cgrady@… added

by oyvind, 16 years ago

More use of import, tests pass

by oyvind, 16 years ago

populate app_label in templatetags/init..py, import_library is own function

by oyvind, 16 years ago

better naming of variables, some code fixes, added some docstrings

by oyvind, 16 years ago

All tests pass

comment:5 by Marc Fargas, 16 years ago

#3349 talks about ImportError in template loading.

comment:6 by oyvind, 16 years ago

This patch shows the tried modules, should the message be less specific, and ask about apps in INSTALLED_APPS as in #3349?

by oyvind, 16 years ago

removed os import

comment:7 by Marc Fargas, 16 years ago

I prefer to know the tried modules, just pointed out #3349 in case it gets invalidated whe this one gets in.
But I doubt it now as I changed #3349 summary (as the patch fixed other places with obscure errors).

comment:8 by oyvind, 16 years ago

Owner: changed from nobody to oyvind

comment:9 by oyvind, 16 years ago

Patch needs improvement: unset
Status: newassigned

comment:10 by Ramiro Morales, 16 years ago

See also #6579 regarding the use of [''] as the 4th. parameter to ___init__() (with some notes from the Python developers advising against its use.)

by oyvind, 16 years ago

fixed import to get the correct module, moved defaulttags and defaultfilters to avoid circular imports, fixes #6579

by oyvind, 16 years ago

forgot to remove some files in the commit, again

by oyvind, 16 years ago

new loader using imp, may need some cleaup, and decide if add_to_builtins should change, and if a import should use a list instead of the getattr solution

by oyvind, 16 years ago

removed import using getattr

by oyvind, 16 years ago

Final patch, i hope :)

comment:11 by Malcolm Tredinnick, 16 years ago

Owner: changed from oyvind to Malcolm Tredinnick
Status: assignednew

comment:12 by Alex Gaynor, 16 years ago

milestone: 1.0 betapost-1.0

1.0 beta has shipped and this was not fixed, will be handled post-1.0.

comment:13 by Malcolm Tredinnick, 16 years ago

milestone: post-1.01.0

I realise people are only trying to help, but can we have a little restraint with triaging tickets off the feature list? Particularly when a maintainer is currently the assigned person. This is still on the 1.0 track. I can do it in a more-or-less backwards-compatible way, with luck.

comment:14 by Malcolm Tredinnick, 16 years ago

milestone: 1.0
Owner: Malcolm Tredinnick removed
Patch needs improvement: set

comment:15 by anonymous, 15 years ago

milestone: post-1.0

comment:16 by Ramiro Morales, 15 years ago

#6579 was related and marked as duplicate.

comment:17 by Antti Kaihola, 15 years ago

#9413 is a symptom of this and has been marked as duplicate.

comment:18 by matehat, 15 years ago

Just a suggestion: if an app (say called 'chunkies') needs only one templatetag module, it would be better to put its content in the 'templatetags/_ _init_ _.py' module instead of forcing it into a new file, say 'templatetags/chunkiestags.py'. This way, we would be able to load it with {% load chunkies %} instead of {% load chunkiestags %}. I looked at the patch and it would only require a short change in 'django/template/_ _init_ _.py'

    if module.endswith("%s.%s" % (library_name, 'templatetags')):
        taglib_module = module
    else:
        taglib_module = str('%s.%s' % (module, library_name))

comment:19 by matehat, 15 years ago

Cc: mathieu.damours@… added

comment:20 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:21 by Jacob, 15 years ago

milestone: 1.1 beta

comment:22 by Malcolm Tredinnick, 15 years ago

milestone: 1.1 beta

No actual progress on this since 1.0 (yeah, we suck). We'll get to it one day, but not this time around.

comment:23 by groks, 15 years ago

Cc: sdeasey@… added

comment:24 by mrts, 15 years ago

milestone: 1.2

Re-targeting for 1.2.

comment:25 by Andrew Badr, 14 years ago

Owner: set to Andrew Badr

can take a look for 1.2

comment:26 by Andrew Badr, 14 years ago

Oyvind, or anyone: how do I use the latest patch? What's up with the /dev/null stuff?

in reply to:  26 comment:27 by Ramiro Morales, 14 years ago

Replying to andrewbadr:

What's up with the /dev/null stuff?

Download the patch using the 'Original Format' link at the bottom of the page and examine it with your preferred text editor. The fact you see /dev/null files is because of an interaction between a) Behavior of some tools (like Git, Mercurial, ...?) that generate diffs containing a /dev/null path for the original file in the header when a new file has been added and b) Trac patch HTML renderer that picks and shows that specific header piece.

Patch should apply cleanly using GNU patch though.

by Andrew Badr, 14 years ago

comment:28 by Andrew Badr, 14 years ago

Here's a version of new_template_lib_loader_14.diff updated to work against trunk. This is my first time using git for patches so hopefully I didn't mess anything up.

Two important notes:

  • The defaulttags module isn't just moved; it is patched and moved at the same time. There is a one-line difference in the "load" function.
  • The templatetag admindocs break. I'll be getting to that later tonight. It was previously pretty tightly coupled to the internals, so I'll assume that's ok for the new version if I can't find a better solution.

It would be great if a committer could confirm that this patch's general approach is correct.

comment:29 by Andrew Badr, 14 years ago

If you are looking at this patch...

  • Should get_templatetags_modules be using import_module or something besides a manual import? I don't know too much about that stuff.
  • The division of code feels wrong. I want to at least move get_templatetags_modules from django.templatetags to django.template (the only place it's used) and maybe even rename it to "get_templatelibrary_modules" (matching the terminology used in django.template). At that point, the django/templatetags folder would only contain libraries, so I'd want to move the directory to django/template/templatetags. (The first change has a stronger case.) Thoughts?

comment:30 by Russell Keith-Magee, 14 years ago

@andrewbadr - Yes, you should be making better use of importlib.import_module. Essentially, any code where you are using imp directly and splitting on '.' - replace it with a call to import_module('full.qualified.module.name')

You need to be careful about moving big blocks of code around, though. The template tests currently fail, for example, because "from django.templatetags.defaultfilters import stringfilter" no longer works. I have plenty of code that imports and uses templates filters as functions. These tests may still have been passing for you if you had a stale .pyc file. django.template may not be the best place for this code, but its a location with historical significance; it can't be changed lightly.

I'll upload an updated patch shortly, which I'll probably commit tomorrow when my head is a little clearer. Other than the import_module and code relocation issues, I'm happy with the approach that has been taken.

by Russell Keith-Magee, 14 years ago

Attachment: t6587-r12291.1.diff added

RC1 of template load refactoring

comment:31 by Andrew Badr, 14 years ago

Cc: andrewbadr.etc@… added

Before committing, note the admindoc breakage I mentioned above and that Øyvind should get credit for writing the patch (I just translated it to trunk). I can work on the admindoc issue tomorrow.

comment:32 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: newclosed

(In [12295]) Fixed #6587 -- Removed nasty path hacking in templatetag loading. Thanks to Øyvind Satvik and Andrew Badr for their work on this patch.

comment:33 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top