Added custom template loader to CMS in order to enable {% include %} and {% import %} in jinja temp…

Added custom template loader to CMS in order to enable {% include %} and {% import %} in jinja temp…

[Wladimir Palant, 2013-12-11 18:46:02]

Nice having somebody other than me work on this.

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
File sitescripts/cms/sources.py (right):

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
sitescripts/cms/sources.py:267: class SourceLoader(jinja2.BaseLoader):
I don't think that Jinja-specific functionality belongs into sources.py - this
should be a private class of TemplateConverter, inside converters.py.

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
sitescripts/cms/sources.py:273: return self.source.read_file(template +
'.tmpl'), None, None
I think this should be self.source.read_include(template, "tmpl") - we want to
use paths relative to the includes directory.

[Sebastian Noack, 2013-12-11 22:20:14]

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
File sitescripts/cms/sources.py (right):

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
sitescripts/cms/sources.py:273: return self.source.read_file(template +
'.tmpl'), None, None
On 2013/12/11 18:46:02, Wladimir Palant wrote:
> I think this should be self.source.read_include(template, "tmpl") - we want to
> use paths relative to the includes directory.

I've already addressed that, but I would like to undo that change, because the
template loader isn't only used for includes but also for imports and extends
(template inheritance). While you can argue whether imports are some kind of
includes, extends definetly aren't. Templates you extend from can and often will
also be rendered on their own. Beside that putting templates into the includes
subdir will also make them available for <? include ... ?>. But there is no
point in including templates that behave as library (only provide variables and
macros that can be imported) or that exist in order to extend them. What do you
think?

[Wladimir Palant, 2013-12-12 07:16:54]

LGTM with the read_file => read_include change reverted.

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
File sitescripts/cms/sources.py (right):

http://codereview.adblockplus.org/6439145228468224/diff/5629499534213120/site...
sitescripts/cms/sources.py:273: return self.source.read_file(template +
'.tmpl'), None, None
On 2013/12/11 22:20:14, sebastian wrote:
> I've already addressed that, but I would like to undo that change, because the
> template loader isn't only used for includes but also for imports and extends
> (template inheritance).

Yes, you are right. We might want to extend the default template meaning that we
will have to address a template in the templates folder. Ok, let's have the
paths relative to the root here.

Nevertheless, the template with the team data should be located in the includes
directory.

[Wladimir Palant, 2013-12-12 12:51:05]

http://codereview.adblockplus.org/6439145228468224/diff/5766466041282560/site...
File sitescripts/cms/converters.py (right):

http://codereview.adblockplus.org/6439145228468224/diff/5766466041282560/site...
sitescripts/cms/converters.py:129: return
re.sub(r'(?:<|\&lt\;)\?\s*include\s+([^\s<>"]+)\s*\?(?:>|\&gt\;)',
resolve_include, text)
Nope, we really don't want to do this in raw HTML for example - localizers
shouldn't be able to insert include directives. This is a temporary hack for
Markdown files only, it should be part of the Markdown converter. You can put it
after the remove_unnecessary_entities step, something like:

  result = re.sub(r"&lt;(\?[^<>]+\?)&gt;", r"<\1>", result)

[Sebastian Noack, 2013-12-12 13:54:39]

http://codereview.adblockplus.org/6439145228468224/diff/5766466041282560/site...
File sitescripts/cms/converters.py (right):

http://codereview.adblockplus.org/6439145228468224/diff/5766466041282560/site...
sitescripts/cms/converters.py:129: return
re.sub(r'(?:<|\&lt\;)\?\s*include\s+([^\s<>"]+)\s*\?(?:>|\&gt\;)',
resolve_include, text)
On 2013/12/12 12:51:06, Wladimir Palant wrote:
> Nope, we really don't want to do this in raw HTML for example - localizers
> shouldn't be able to insert include directives. This is a temporary hack for
> Markdown files only, it should be part of the Markdown converter. You can put
it
> after the remove_unnecessary_entities step, something like:
> 
>   result = re.sub(r"&lt;(\?[^<>]+\?)&gt;", r"<\1>", result)

I agree that we should do that only for markdown. But I went for an other
approach, which doesn't require another regex to apply and having the include
syntax defined in two places.

[Wladimir Palant, 2013-12-13 14:01:45]

LGTM even though I don't really think that this is the best solution (see
below).

http://codereview.adblockplus.org/6439145228468224/diff/5766466041282560/site...
File sitescripts/cms/converters.py (right):

http://codereview.adblockplus.org/6439145228468224/diff/5766466041282560/site...
sitescripts/cms/converters.py:129: return
re.sub(r'(?:<|\&lt\;)\?\s*include\s+([^\s<>"]+)\s*\?(?:>|\&gt\;)',
resolve_include, text)
On 2013/12/12 13:54:39, sebastian wrote:
> I agree that we should do that only for markdown. But I went for an other
> approach, which doesn't require another regex to apply and having the include
> syntax defined in two places.

The other regexp applied to processing instructions in general and only fixed
that one bug - in a single line. You've implemented a more complex solution now
which IMHO is an overkill for a temporary workaround.

Whatever, I guess I can live with that solution as well...