Hi Preston, On 06/10/2015 11:46 AM, Preston Timmons wrote: > I've been working through solutions for #15667 -- template based widget > rendering. This is a problem that was close to a solution at one time, but > stalled out due to performance concerns and difficulties with defining a > workable API to create configurable template loaders. > > Now that Jinja2 is supported, performance isn't as much a concern. So, > I'd like to tackle the API portion of this.
Thanks for picking this up again! > This problem has two sides: > > 1) Converting individual widgets to be rendered with templates. > 2) Deciding how to instantiate a user-customizable template engine. > > The first is easy to accomplish, but the second isn't. I've written a > proposal for the second problem available here: > > https://gist.github.com/prestontimmons/24a2a835bea590afb70b This proposal looks excellent. A few comments: 1) I think the new `renderer` argument to `Widget.render()` should be kept optional, and BoundField should have a fallback to not pass it in if not accepted, in order to avoid requiring immediate backwards-incompatible updates to custom widgets' render() methods. The Widget.render() method's signature is documented, so it is covered by the backwards-compatibility policy. The BoundField calling fallback can have a deprecation path; keeping the renderer arg optional forever (with fallback to default renderer from settings) doesn't seem like a terrible idea. 2) I don't think that looking up the magic name 'forms' in `TEMPLATES` is a good idea for overriding the forms template loader. I think it would be better to just require using a custom renderer (which can be a simple subclass of the built-in TemplateRenderer) for that case. 3) Regarding the template-precedence conundrum you mentioned: IMO the best end-result here is to simply add 'django.forms' to `INSTALLED_APPS`. This provides full flexibility (you can order `INSTALLED_APPS` however you like to override with templates from a different app, or of course override with templates in `DIRS`) without requiring any new concepts. So then the question is just a backwards-compatibility path for getting 'django.forms' into `INSTALLED_APPS`. Here's what I'd propose: The default `TemplateRenderer` has an `engine` cached-property, with the following behavior: a) If 'django.forms' is in `INSTALLED_APPS` and there is at least one configured template engine with `APP_DIRS: True`, return `None`. b) Otherwise, fallback to instantiating and returning a simple Jinja2 backend (similar to the `default_backend` in your gist, but I think even more stripped down, with `APP_DIRS: False`). Raise a (pending) deprecation warning. If `TemplateRenderer.engine` is `None`, then `TemplateRenderer.render` would use `django.template.loader.get_template` (thus using the same engine fallback strategy as normal template rendering from a view). Document in the form-rendering docs (and in the release upgrade notes, which are linked to by the above deprecation warning) that if you want to override the default form widget templates, you need to include 'django.forms' in `INSTALLED_APPS` and make sure you have at least one template engine with `APP_DIRS: True`. Overriding widget templates is a new feature, so it's OK if it requires explicit opt-in. Once the deprecation warning runs its course, the default `TemplateRenderer.engine` property would simply return `None` in all cases (unless we add an `engine_name` class attr; see below), thus deferring to normal template rendering, and there would be no automatically-generated special template engine for forms. I think this provides a full backwards-compatible path for the simplest case, while still preserving all the flexibility you could want. If you don't want the built-in templates at all, and want to provide all your own, you don't need `APP_DIRS: True` or 'django.forms' in `INSTALLED_APPS`. If you want to only use a specific template engine for form widgets, you can subclass `TemplateRenderer` and override the `engine` property to return your engine (which could be instantiated right there in the renderer, or could be from the `TEMPLATES` setting and fetched in the renderer using `get_engine`). To make the latter customization even simpler, we could add an `engine_name` attribute on `TemplateRenderer`, defaulting to `None`, but if set to anything other than `None`, the `engine` property would call `get_engine` with that name. Then the subclass only has to define the `engine_name` attribute to enforce use of a specific engine. Or if that type of customization turns out to be very common, we could even go one step further and add a `FORM_RENDERER_TEMPLATE_ENGINE` setting which can be set to a template engine name, to avoid needing to subclass `TemplateRenderer` at all. (My feeling is we should probably do the `TemplateRenderer.engine_name` thing, but not the setting.) > In addition, a WIP branch is available here: > > https://github.com/prestontimmons/django/tree/ticket-15667 Can you turn this into a PR marked [WIP] and link it from the ticket? Having a PR, even if its not merge-ready yet, makes code review and commenting much easier. > Hopefully, we can finally get this in. :) +1! Carl -- 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 django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. 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/5578A931.8010106%40oddbird.net. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: OpenPGP digital signature
