Hi Preston, On 14 janv. 2015, at 20:43, Preston Timmons <[email protected]> wrote:
> 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. Good. > 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 Perhaps we’ll bikeshed names at some point but let’s leave the “naming things” problem aside for now. “name” must be optional for loaders that don’t load templates from files. “loadname” must be optional too for templates instantiated from strings. Here optional means that things don’t break if the value is None. > Third, loaders implement these methods: > > get_template_sources -> yields origins > get_internal -> takes an origin and returns the template contents I don’t like the code duplication between get_internal and load_template_source. I assume your design ended up that way because you needed get_internal and load_template_sources will be deprecated. Is this correct? > 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. TemplateDoesNotExist is used both internally by the DTL and in generic APIs such as django.template.loader.get_template. If it’s designed to accept a list of tried origins, then we need: - a generic Origin without engine, loader, status and tried (at least) - a DTL Origin as described above. Does this make sense? > 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. Yes. Maintaining compatibility with LoaderOrigin as documented in 1.7 appears to be a lost cause anyway. > 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. I think the appropriate way for origins to return template code would be to call back into the template loaders. Reading template contents in the job of the loader. It should be possible to write a single origin class that works for most loaders. The sole purpose of the reload() method is to display tracebacks in templates. This logic should be moved out of the debug view and into the template backends. See #24119. As a consequence, we should leave it outside of this refactoring and simply preserve it for DTL origins until that ticket gets fixed. > 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. I’ll take your word on this. I haven’t spent enough time working on the implementation to suggest an alternative. > 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? If you’re going to change both its arguments and its return type, you can change the method's name :-) The second solution looks better to me. Hopefully we can write a “just do this” guide to help people maintaining custom template loaders upgrading. > 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. Yes. -- Aymeric. -- 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/889CD261-B605-4C29-BCF4-FACB710975A1%40polytechnique.org. For more options, visit https://groups.google.com/d/optout.
