Hi Aymeric, I got a chance to update my patch with the use of origins. The good news is that it's simpler than the old implementation. I have a few api questions below. The updated branch is here for now:
https://github.com/prestontimmons/django/compare/ticket-15053-origin?expand=1 As a quick recap: First, this branch uses origins everywhere: for extends history, for TemplateDoesNotExist errors, etc. There's no more 4-tuple of statuses. Second, I use these attributes on the origin: engine loader loadname -> template name name -> full template path status -> Skipped, Found, Source does not exist tried -> a list of origins tried up to this one reload -> gets template contents __eq__ __hash__ -> added so the cache loader can use origin as a cache key Third, loaders implement these methods: get_template_sources -> yields origins get_internal -> takes an origin and returns the template contents Fourth, TemplateDoesNotExist takes an optional tried argument which is also a list of origins. This will be used in the debug views. I didn't change the debug views in this branch yet, though. *Questions* *Origin attributes* I'd like to do some renaming: origin.loadname -> origin.template_name I think it's hard to remember the difference between origin.name and origin.loadname. I think "template_name" is more conventional. origin.reload -> origin.read If the method to read a template contents lives on the origin, I chose to prefer using the origin method rather than the loader method directly. There's not much consequence here, but it means it's no longer a special method just for the debug view. *tried and status* The nice thing about putting the tried and status on origin are that it works well with a recursive api. It even means I could get rid of the extra context variable used to store extends history. The sucky things are that tried and status are set and manipulated outside the origin constructor. The cached loader also has to be sure to reset the origin before returning the cached template. That might be warranted, but part of me wonders if it's lousy api. *Deprecation path* If we want to maintain the load_template api, I can change it to accept the skip argument. Within load_template I can add a hasattr check for "get_internal". If it exists, use the new code. Otherwise, use the old "load_template_sources" method. Since adding a kwarg to load_template could potentially break 3rd-party loaders that override it, I could add an _accepts_skip_kwarg property to base.Loader like you've done with _accepts_engine_in_init = True. This would enable Engine.find_template to exclude that if necessary. If we do this, I'd like to deprecate the (template, template_origin) return value in favor of just template. Returning a tuple is redundant. Ditto for Engine.find_template. If we use get_template instead, we don't need to deprecate the return value and we don't need _accepts_skip_kwargs. Instead we can just recommend use of the new method until load_template is removed. Either way, load_template_sources is obsoleted. Which deprecation path do you think is preferable? *Reading template contents* If we have an origin.read() function, an option to consider is updating each loader to return a custom origin object rather than adding "get_internal" or "load_template_source" to the loader. These could be something like FileOrigin, EggOrigin, CacheOrigin, DbOrigin, etc. I can't think of any deeper advantages this holds, though. So I leave it at that. *Adding skip to Engine.get_template/Engine.find_template* I wondered whether skip should also be added to Engine.get_template. Doing so eliminates some looping logic from Engine.find_template that is reimplemented in the extends node. After implementing it though, I've decided against it: 1) In general, users don't work with origins directly. Therefore I don't think it has much applicability outside the internal loaders. 2) It takes quite a bit of fiddling around to maintain a linear list of templates that were found or skipped. Even though the new implementation removed some duplication, it also wasn't any simpler than the old. *Conclusion* If the api so far makes sense, I'll update the debug view post-mortem messages and add in the docs and deprecation warnings. Thanks, Preston -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/70225dbc-0316-4c5f-9e13-7297df860d78%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
