Hello Christopher, Since I opened that ticket and sent you down this rabbit-hole, I can at least offer some thoughts :-) In full disclosure, I should also say that since opening the ticket I've had doubts about whether this change (although clearly an improvement in the abstract) is worth the effort and disruption to the codebase, especially given the existence of Jinja2, a standalone (and global-state-free) implementation of a template language quite similar to Django's (but much faster).
I don't want to discourage you from working on the project if you find it interesting and informative, and it may well be that the results are worth the effort and end up being merged into Django; but it is also possible that we look at the patch and say "sorry, that's just too much disruption for the benefits" - so you should be prepared for that possibility. I really don't have a good sense of where the other core devs stand on how much this type of deep refactoring is worth. On 01/28/2013 09:30 AM, Christopher Medrela wrote: > 4. What I did so far? I created base.TemplateEngine class and > base.default_engine global instance. Some functions (mostly from loader > module: get_template, find_template, select_templates and from base > module: add_to_builtins, get_library, compile_string) delegates to > appropriate methods of default_engine. I also partially rewrote tests > (only tests/regressiontests/templates/tests.py) so they don't use > default_engine directly. That's an impressive amount of progress! I've looked through the code a bit; it's obviously not complete, but the direction looks right to me. > 5. The one issue is that the engine is needed everywhere. For example, > consider creating base.Template instance. At the time of creation the > template source is being compiled. The template may load a library, so > the compilation cannot be done without list of all libraries defined in > the engine. To sum up, we cannot just type Template(source), because > this depends on template engine. I solved this issue by passing engine > instance to Template constructor. In order not to break everything, I > renamed Template class to _Template and created function called Template > that does "return _Template(default_engine, **kwargs)". Yes, I'd expect that in this approach most other classes will need a reference the engine to do their work. I think this should be accepted and just done thoroughly. I think your backwards-compatibility approach with Template is generally correct, though I'm not sure _Template is a good name for what in the long run ought to be the primary public implementation of the class. > 6. The same issue occures when you consider including another template. > loader_tags.IncludeNode needs to be able to load another template. I > resolved this by passing an engine to base.Parser constructor. A parser > passes itself to constructors of nodes, so IncludeNode can load another > template in this way: "parser.engine.find_template(...)". Makes sense to me. > 7. Then there is the issue of loaders. At the first, I thought that > passing the engine will be not necessary if I change behaviour of > loader.BaseLoader.load_template method so it will always return source > of template instead of Template instance. So I did it for BaseLoader. > Few days ago I did the same for loaders.cached.Loader, but I realised > that this change means no caching at all -- the loader cached compiled > templates. Now it caches nothing. The current architecture let us to > cache template sources but not compiled templates. > > 8. There are two solutions. First, we can just pass the engine to every > loader. Second, we can remove cache-loader and implement caching inside > TemplateEngine. Both solutions are ugly. Do you see any better one? I think loaders will need to have access to the template engine that is using them (though for backwards-compatibility they may need to default to default_engine if not given another engine). > 9. Now, there is another issue. Changing API. It's obvious that changing > base.Parser signature is OK since it's an internal structure. It's also > easy to notice that base.Template is used everywhere so we cannot change > API of Template. But there are many places where it's unclear if a > method or class is public (it means that we promise we won't change its > signature) or internal. For example, I don't know how much we can change > loaders. I don't know if Django developers types "isinstance(obj, > Template)" -- it won't work know. I don't know if I can change signature > of SsiNode as I did. By the way, I think that we should make clear > distinction between these two methods by using underscore convention. Historically Django's codebase hasn't made much use of initial underscores. Instead, the practice has been that backwards compatibility promises apply only to documented classes and methods. So I think you can use the documentation as your guide here; if something is not documented, generally it can be changed. > 10. As I said I'm trying to rewrite tests from tests.py file so they > won't use the default engine. There left some sources of dependency on > default engine. One of them are include tags in tests which load > templates at the time of importing module [3] (or the source of problems > is in base.Library.inclusion_tag?). Another one are template.response > module [4] and all other test files (i. e. callables.py, custom.py). I > didn't dig into these issues deeply yet since I wanted to publish my > results and take some feedback. For the first case, I would say those tests should simply be changed to not load templates when the module is imported. As for TemplateResponse, it would need to use default_engine, though I suppose it could sprout an additional instantiation parameter to accept an alternative engine. > 11. A small issue is test > regressiontests.views.tests.debug.DebugViewTests.test_template_loader_postmortem. > The problem is in method > django.views.debug.ExceptionReporter.get_traceback_data. It uses > template_source_loaders global state which was removed. We can just use > default_engine. I wonder when a Django developer uses a custom template > engine instead of the global default one then they will get faulty error > message. Is it important or negligible? It looks to me on brief inspection like the TemplateDoesNotExist exception instance would need to carry with it the necessary debugging info that is currently pulled out of the global template_source_loaders. > 12. As I said there is another source of global-state -- django > settings. I think we can deal with that by passing all necessary > settings (like TEMPLATE_DEBUG, INSTALLED_APPS etc.) to TemplateEngine > constructor. It means that we won't be able to create the default engine > at the module-level since settings are not configured at the time of > importing template package. We will have to make instancing default > engine lazy. It means that there should be get_default_engine() function > which creates the default engine at the very first call. I wonder if we > will fall into circular imports hell or other problems... I think you're right that either instantiation or internal initialization of the default_engine will need to be lazy so that settings are not accessed at module-scope. This is irritating, but doable. > 13. At the end I would like to split the template package into new > django-independent package (called i. e. 'template_engine') and > django-specific package (named 'templates') which defines default_engine > and all django-specific tags and filters. By django-independent I mean > not depending on settings neigher other django modules excluding utils. > I won't split tests in the same way - I think there is no need for that. This makes sense in terms of code hygiene, though if you're thinking of the "template_engine" as something that is likely to see significant use outside of Django, my feeling is that the existence of Jinja2 makes this unlikely. > 14. Is this plan good? What issues do you see? Could you look at my work > and review it (I wonder especially if I'm using a python feature not > supported by python version supported by Django?). Best way to notice this is to fix up the test suite into a running state, and run it with the supported Python versions (minimum is 2.6 currently, the more likely area for problems is Python 3, since Django master now supports Python 3). If you have some > experience at splitting Django into smaller parts, please share it with > me. Tell me what I'm doing wrong (and what's good). If you are looking > for having this ticket done, please tell me what is more important and > what is negligible, what API do you expect the template engine? If you > were working at the ticket, how would you complete the ticket? I hope > that we can make Django better. The approach you're taking is very much the approach I'd envisioned when I opened the ticket, and it seems like you're thinking through all the potential issues carefully; I don't see any obvious problems in your approach. As I said above, I've since felt like this problem is perhaps more work than it is worth at this point in Django's evolution (for myself, anyway), but I won't speak for anyone else on that score, others may feel differently. Carl
signature.asc
Description: OpenPGP digital signature
