Conversation
06d1bc7 to
35a2149
Compare
|
As currently implemented, updating a project for this change requires either installing Jinja2 or configuring a @MoritzS, if you'd like to contribute any improvements to the jinja2 templates as suggested in your comment on the original PR, that would be welcome. |
|
I looked over the jinja2 templates again: To really make them look nicer, the |
b34ce36 to
22af1cd
Compare
|
@timgraham, about the |
django/forms/renderers/templates.py
Outdated
| if templates_configured(): | ||
| return | ||
|
|
||
| if not jinja2: |
There was a problem hiding this comment.
this logic should maybe just go inside the default_engine static method?
1c26943 to
75e9dd4
Compare
|
I'm still planning to push the changes from carljm@7d734cf plus tests (and the template-not-found chaining fix that @prestontimmons proposed), but I've been a bit distracted with DEP 5 stuff and haven't gotten to it yet. Hopefully tonight. |
|
@timgraham I did just notice one html-escaping test failing in Re the public renderer API, although for |
f4c3164 to
f99b933
Compare
25cdb28 to
a9464fd
Compare
f8e1818 to
0ae75ca
Compare
RendererMixin will soon be removed but this removal and the corresponding test changes stand on their own.
|
I think this is ready for review. Not sure if the new |
carljm
left a comment
There was a problem hiding this comment.
Looks good, modulo a few comments! I didn't re-review all the widget stuff, just the core renderers code and its docs.
I don't mind adding post-app-dirs template dirs as a feature of template backends, but I think it would be both simpler to understand and implement, and actually more powerful, to introduce POST_APP_DIRS as a separate list of dirs, rather than a boolean SEARCH_APP_DIRS_BEFORE_DIRS.
django/forms/renderers/templates.py
Outdated
| }) | ||
|
|
||
|
|
||
| class ProjectTemplateRenderer(BaseTemplateRenderer): |
There was a problem hiding this comment.
This renderer probably also deserves a docstring?
django/template/backends/base.py
Outdated
| self.name = params.pop('NAME') | ||
| self.dirs = list(params.pop('DIRS')) | ||
| self.app_dirs = bool(params.pop('APP_DIRS')) | ||
| # This option ended up working only for Jinja2 because DjangoTemplates |
There was a problem hiding this comment.
This comment seems confusing/misleading. AFAICT this option works and is used for both Jinja2 and DTL engines, although in different ways/locations; DTL just passes it down to the Engine.
docs/ref/forms/renderers.txt
Outdated
| The low-level render API | ||
| ======================== | ||
|
|
||
| Widget templates are stored in the ``django/forms/widgets`` path. A project |
There was a problem hiding this comment.
The second sentence is slightly misleading. "A project" cannot by default override a widget template (unless the default form renderer is changed to ProjectTemplateRenderer), though an installed app can do so. I think just changing "A project" to "An installed app" would preserve the main point while being technically accurate.
There was a problem hiding this comment.
Correct, although I'm concerned that it gives the idea that third-party apps should be overriding templates to customize things when that's really not appropriate since only the template in the earliest installed app will be found. Do you agree that guideline?
There was a problem hiding this comment.
Yeah, I agree that third-party apps in most cases shouldn't be overriding form widgets (though I suppose I could see some exceptions for e.g. something like a django-bootstrap-forms app, if it clearly advertises that this is what it does).
Maybe it'd be possible to rephrase this sentence in a way that doesn't have to mention either "project" or "app", since the real point here is just that form widgets can be overridden. Maybe passive voice? Or do we use second person in the docs? "You can provide a custom template for..."
docs/ref/forms/renderers.txt
Outdated
| #. Adding the built-in widgets templates directory (``django/forms/templates`` | ||
| or ``django/forms/jinja2``) in ``DIRS`` of one of your template engines. | ||
|
|
||
| You've chosen to take full control, so it's your responsibility to make |
docs/ref/forms/renderers.txt
Outdated
|
|
||
| .. module:: django.forms.renderers.templates | ||
|
|
||
| .. class:: BaseTemplateRenderer |
There was a problem hiding this comment.
I'm not sure that we should document BaseTemplateRenderer or EngineRendererMixin; they seem like internal implementation details to me. I think any hypothetical value they'd provide to the implementor of a custom renderer (which is minor, they don't do very much) is outweighed by the added "ravioli code" complexity introduced by inheriting from built-in mixins and base classes. If the built-in renderers don't suit, the implementor of a custom renderer is likely to need to modify the code provided by these classes anyway, and I think we do them a better service by encouraging them to just implement what they need directly.
Documenting these also reduces our refactoring flexibility in the future.
|
|
|
The current implementation of the filesystem loader (looking at Can of course make the new parameter optional and fall back to |
37ae6c9 to
149eddf
Compare
carljm
left a comment
There was a problem hiding this comment.
Looks good to me! As before, I reviewed the core renderers code, tests, and docs; I didn't look over all the widget changes in detail. I don't think those have changed recently?
django/forms/renderers.py
Outdated
| from django.utils.functional import cached_property | ||
| from django.utils.module_loading import import_string | ||
|
|
||
| from . import __file__ as forms_file |
There was a problem hiding this comment.
What is the benefit of this import over just using our own __file__ directly? If I'm not mistaken, one would result in .../django/forms/__init__.py and the other would result in .../django/forms/renderers.py, which is irrelevant since either way we're about to call os.path.dirname(...) on it. What am I missing?
docs/ref/forms/renderers.txt
Outdated
|
|
||
| To use this backend, all the widgets in your project and its third-party apps | ||
| must have Jinja2 templates. You can't use this renderer, for example, if you're | ||
| using :mod:`django.contrib.admin` because Jinja2 templates aren't included for |
There was a problem hiding this comment.
Is it worth adding "(unless you provide Jinja templates for all the admin widgets yourself)" here?
| os.path.join( | ||
| upath(os.path.dirname(__file__)), | ||
| '..', | ||
| getattr(self, 'expected_widget_dir', 'templates') + '/forms_tests/custom_widget.html', |
There was a problem hiding this comment.
Why not just set expected_widget_dir = 'templates' as a class attr on SharedTests and avoid the need to do this getattr twice?
|
So, bare with me, I just got another completely stupid idea. But before I come to it, lets take a step back: The current renderer system is all nice and well but it cries for user problems (speaking with my IRC support hat on) -- ie why is the Django form renderer not picking the default template settings like So what I came up with is the following: Only ship a A second option which follows naturally is that we could even consider dropping the renderers completely and only use With the second option we would then achieve the following:
EDIT:// And best of all: If we really need the renderers in the future, we can just readd them ;) |
|
@apollo13 Most of the ideas in your proposal already came up at one time or another in the long discussion of the options here :-) A few problems:
I think you may be right, though, that people will find the default renderer (as currently coded) to be a source of confusion, because it's close to the same as a typical Also, I just realized that allowing app overrides in the default renderer is a potential backwards-compatibility issue; people may already have an installed app that unintentionally/accidentally "overrides" a template path we are using for a form widget. Not likely, maybe, but possible. All of which makes me think that @timgraham was right about not allowing apps to override form templates in the default renderer. In that case IMO we should treat the default renderer as basically a back-compat shim, with the intent being that most people will eventually be best off switching to |
The wrench in this plan is the admin/gis widgets. They won't be discoverable by default if app directories aren't searched. |
|
Not overriding isn't the same thing as not searched. Tim's proposal was to include app dirs in the default renderer engine, but put the core forms dir first.
|
|
@carljm I feared as much, I just recently read the latest thread on the ML to get a bit of an overview, but on the other hand I also wanted to provide a fresh new perspective without being influenced too much by the decision process so far to make it easier for my mind to come up with new solutions (I realize that this adds extra work for you, which is probably not fair, so I'll just try to answer your last post and if you all think I am still on the wrong track, I'll leave it be). Sorry in advance for any extra work I cause(d). Since you've split your arguments so nicely I'll try to answer them one by one -- they don't seem to overlap that much anyways
And please lets not sweep that under the rock, the first question in IRC I (and everyone else in there) will have to answer for weeks will be:
Yeah, screw that :D (All our approaches to backwards compat have usually been like: "it works in 99% of the cases, but if you have that one obscure config, you have to change a little bit, sorry")
This is a very good question. And honestly something we need to think more about, cause currently if you start overriding widgets, it will affect the admin too, which is not something you want (since the admin doesn't override all widgets but only some) and most likely should stay in the design we provided. If we do not allow those overrides we can add the django.forms template directory as last entry in
|
|
On the last point, the admin doesn't override any widgets. All its custom widgets are namespaced such as |
|
@apollo13 No worries, taking a look with a fresh mindset is good :-) I am not a fan of the I agree with your support concern, but we've been around and around and around this issue: the only way to fully address it would be to somehow use full I think the only feasible option for reliable forms backward-compatibility is what this PR does: default to a simple template engine we construct ourselves that we are sure can definitely load the built-in and contrib-app widget templates. And the best we can do to address the support concern that may arise once people start to use custom widgets and wonder why their I'm really fine either way on the question of whether we use |
|
Mhm, guess I'll rest my case -- If we are going to promote the usage of |
5a3fc9e to
8c581a4
Compare
carljm
left a comment
There was a problem hiding this comment.
Some doc nits, otherwise lgtm.
docs/ref/forms/renderers.txt
Outdated
docs/ref/forms/renderers.txt
Outdated
docs/ref/forms/renderers.txt
Outdated
docs/ref/forms/renderers.txt
Outdated
docs/ref/forms/renderers.txt
Outdated
There was a problem hiding this comment.
It returns the rendered template as a string, it doesn't return a Template object.
docs/ref/forms/renderers.txt
Outdated
1a96b54 to
d88281a
Compare
Thanks Carl Meyer and Tim Graham for contributing to the patch.
Updated from #4848.
https://code.djangoproject.com/ticket/15667
https://groups.google.com/d/topic/django-developers/TmHdj5SCGNk/discussion