Issue 5044 - Support absolute paths for Jinja templates

Issue 5044 - Support absolute paths for Jinja templates

Repository: https://hg.adblockplus.org/sitescripts/
Base revision: add7e39f7e4d

[Vasily Kuznetsov, 2017-03-30 17:35:10]

This patch enables support for using absolute paths for Jinja templates anywhere
in sitescripts (it's needed for #5044 but I think it makes more sense to make it
generic). Testing that this won't break anything will be somewhat tricky because
many paths are taken from the config file and if they were starting with a "/",
the semantics might change. In the example config all paths are relative but we
also need to check the configs that live on the real servers where this stuff
will be deployed.

I'm cc-ing Matze on the review to advise if there are any absolute paths to
templates in the configs that are on production servers. The variables to check
are:

[extensions]
changelogTemplate
safariUpdateManifest
androidUpdateManifest
geckoUpdateManifest
nightlyIndexPage
*updateManifest

[reports]
htmlDigestTemplate
webTemplate
errorTemplate
showUserTemplate
submitResponseTemplate
mailDigestTemplate
notificationTemplate

[stats]
mainPageTemplate
filePageTemplate
fileOverviewTemplate

[subscriptions]
*Template

[formmail]
template

[send_installation_link]
email_template

[submit_email]
*_verification_email_template

[Sebastian Noack, 2017-03-30 17:41:24]

Adding Wladimir as reviewer. As far as I know, he was the one introducing jinja2
templates in sitescripts, and I barely used them myself here. So if there are
any reasons to not support absolute paths, he should know.

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.py
File sitescripts/utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.p...
sitescripts/utils.py:161: if key not in _template_cache:
Assuming this was the only occurrence of `not x in y` in this file, we can
remove the ignore for the respective flake8 error for this file in tox.ini now.

[Wladimir Palant, 2017-03-30 17:47:42]

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.py
File sitescripts/utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.p...
sitescripts/utils.py:160: key = (template_path, template, autoescape)
This caching key wasn't entirely correct before, and with these changes it's
completely bogus. How about caching by absolute/canonical template path?

[Vasily Kuznetsov, 2017-03-30 18:35:53]

Thank you for the fast response. Here's patch set 2.

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.py
File sitescripts/utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.p...
sitescripts/utils.py:160: key = (template_path, template, autoescape)
On 2017/03/30 17:47:42, Wladimir Palant wrote:
> This caching key wasn't entirely correct before, and with these changes it's
> completely bogus. How about caching by absolute/canonical template path?

Done

https://codereview.adblockplus.org/29398791/diff/29398792/sitescripts/utils.p...
sitescripts/utils.py:161: if key not in _template_cache:
On 2017/03/30 17:41:24, Sebastian Noack wrote:
> Assuming this was the only occurrence of `not x in y` in this file, we can
> remove the ignore for the respective flake8 error for this file in tox.ini
now.

Done

[Sebastian Noack, 2017-03-31 06:33:42]

https://codereview.adblockplus.org/29398791/diff/29398796/sitescripts/utils.py
File sitescripts/utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29398796/sitescripts/utils.p...
sitescripts/utils.py:161: key = (canonical_path, autoescape)
Note that following are NOT equivalent:

  get_template('bar/baz.tmpl', template_path='/foo')
  get_template('baz.tmpl', template_path='/foo/bar')

If baz.tmpl is extending or including another template, the former will expect
to find the other template in '/foo', the latter in '/foo/bar'. So the key
should rather be constructed like that, which seems slightly simpler anyway:

  key = (template_path, template, autoescape)

[Vasily Kuznetsov, 2017-03-31 16:01:48]

https://codereview.adblockplus.org/29398791/diff/29398796/sitescripts/utils.py
File sitescripts/utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29398796/sitescripts/utils.p...
sitescripts/utils.py:161: key = (canonical_path, autoescape)
On 2017/03/31 06:33:42, Sebastian Noack wrote:
> Note that following are NOT equivalent:
> 
>   get_template('bar/baz.tmpl', template_path='/foo')
>   get_template('baz.tmpl', template_path='/foo/bar')
> 
> If baz.tmpl is extending or including another template, the former will expect
> to find the other template in '/foo', the latter in '/foo/bar'. So the key
> should rather be constructed like that, which seems slightly simpler anyway:
> 
>   key = (template_path, template, autoescape)

Since it's not possible to escape Jinja template environment, the only way this
scenario could occur is if template environment B is located inside template
environment A and there's a template that lives in B and works correctly in both
producing different results (because of different includes and/or dependencies).
This seems pretty unlikely (although not impossible), so I understood Wladimir's
comment to mean "the key should just be the canonical path to the template
file". I suppose what he was actually suggesting was to canonicalize the content
of `template_path` variable. I have now done that.

[Sebastian Noack, 2017-04-01 13:26:35]

LGTM

[Jon Sonesen, 2017-04-03 07:42:41]

Looks pretty good just a couple questions on the tests :)

Thanks!

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:22: """Load template from inside sitescripts."""
Perhaps the docstring here should include the additional space before the text
inside of it?

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:29: """Load template using relative or absolute path."""
Same here?

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:36: template = get_template('template.tmpl',
template_path=tmpdir.strpath)
Does this work as relative since you provide the name of the file and it's
directory?

[Vasily Kuznetsov, 2017-04-03 08:59:25]

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:22: """Load template from inside sitescripts."""
On 2017/04/03 07:42:41, Jon Sonesen wrote:
> Perhaps the docstring here should include the additional space before the text
> inside of it?

Thanks for bringing this up. Maybe we should say something about docstring
format in our style guide beyond just following PEP257. Otherwise it's unclear
whether additional spacing around the docstring should be added or not. And the
docstrings look kind of messy when this is not consistent.

For myself I don't see any value in additional spaces around docstrings. It
seems like they only make less text fit into one line. One-line docstring
examples in PEP257 don't have any surrounding whitespace, so I usually just
follow that style.

Does anyone have any arguments for surrounding whitespace?

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:36: template = get_template('template.tmpl',
template_path=tmpdir.strpath)
On 2017/04/03 07:42:41, Jon Sonesen wrote:
> Does this work as relative since you provide the name of the file and it's
> directory?

It's considered relative because the first argument doesn't start with a slash.
Having `template_path` or not doesn't change it (see previous test).

[Jon Sonesen, 2017-04-03 09:03:23]

Regarding the docsrings as mentioned in the comment I brought this up since you
suggested that I change my docstrings on the tests for the review of csv_log for
formmail 2 so I recommend we write down this docstring policy since there is
already inconsistencies arising.

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:22: """Load template from inside sitescripts."""
On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > Perhaps the docstring here should include the additional space before the
text
> > inside of it?
> 
> Thanks for bringing this up. Maybe we should say something about docstring
> format in our style guide beyond just following PEP257. Otherwise it's unclear
> whether additional spacing around the docstring should be added or not. And
the
> docstrings look kind of messy when this is not consistent.
> 
> For myself I don't see any value in additional spaces around docstrings. It
> seems like they only make less text fit into one line. One-line docstring
> examples in PEP257 don't have any surrounding whitespace, so I usually just
> follow that style.
> 
> Does anyone have any arguments for surrounding whitespace?

Well I brought this up because you mentioned that i should surround them in
spaces on a previous review, even thoough i agree with you. So yes, we should
write this down somewhere.

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:36: template = get_template('template.tmpl',
template_path=tmpdir.strpath)
On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > Does this work as relative since you provide the name of the file and it's
> > directory?
> 
> It's considered relative because the first argument doesn't start with a
slash.
> Having `template_path` or not doesn't change it (see previous test).

I see, thanks

[Sebastian Noack, 2017-04-03 09:51:43]

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:22: """Load template from inside sitescripts."""
On 2017/04/03 09:03:23, Jon Sonesen wrote:
> On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> > On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > > Perhaps the docstring here should include the additional space before the
> text
> > > inside of it?
> > 
> > Thanks for bringing this up. Maybe we should say something about docstring
> > format in our style guide beyond just following PEP257. Otherwise it's
unclear
> > whether additional spacing around the docstring should be added or not. And
> the
> > docstrings look kind of messy when this is not consistent.
> > 
> > For myself I don't see any value in additional spaces around docstrings. It
> > seems like they only make less text fit into one line. One-line docstring
> > examples in PEP257 don't have any surrounding whitespace, so I usually just
> > follow that style.
> > 
> > Does anyone have any arguments for surrounding whitespace?
> 
> Well I brought this up because you mentioned that i should surround them in
> spaces on a previous review, even thoough i agree with you. So yes, we should
> write this down somewhere.

I'm used to add leading/trailing new lines to docstrings, because the text and
quotes are aligned, when spanning over multiple lines:

  def foo():
      """Subsequent lines continue in a different column
      which makes it somewhat harder to read"""
      pass

  def foo():
      """
      The first line starts in the same column
      as subsequent lines, forming one block.

      The quotes, opening/closing the docstring use the
      same columns as well, making it easier to identify
      docstrings without relying on syntax highlighting.
      """
      pass

I wouldn't insist sticking to it though, also considering that PEP257 seems to
suggest to omit the extra newlines in its examples. That said, if we go without
the extra newlines, IMO there is no need to adapt the coding practices, as we
already refer to PEP257 there. But we might as well decide for the extra
newlines for the reasons above, if they convince you guys.

[Vasily Kuznetsov, 2017-04-03 10:25:16]

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:22: """Load template from inside sitescripts."""
On 2017/04/03 09:03:23, Jon Sonesen wrote:
> On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> > On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > > Perhaps the docstring here should include the additional space before the
> text
> > > inside of it?
> > 
> > Thanks for bringing this up. Maybe we should say something about docstring
> > format in our style guide beyond just following PEP257. Otherwise it's
unclear
> > whether additional spacing around the docstring should be added or not. And
> the
> > docstrings look kind of messy when this is not consistent.
> > 
> > For myself I don't see any value in additional spaces around docstrings. It
> > seems like they only make less text fit into one line. One-line docstring
> > examples in PEP257 don't have any surrounding whitespace, so I usually just
> > follow that style.
> > 
> > Does anyone have any arguments for surrounding whitespace?
> 
> Well I brought this up because you mentioned that i should surround them in
> spaces on a previous review, even thoough i agree with you. So yes, we should
> write this down somewhere.

Hm. Are you sure? I don't think I would suggest this. Perhaps we've
misunderstood each other.

https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
tests/test_utils.py:22: """Load template from inside sitescripts."""
On 2017/04/03 09:51:43, Sebastian Noack wrote:
> On 2017/04/03 09:03:23, Jon Sonesen wrote:
> > On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> > > On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > > > Perhaps the docstring here should include the additional space before
the
> > text
> > > > inside of it?
> > > 
> > > Thanks for bringing this up. Maybe we should say something about docstring
> > > format in our style guide beyond just following PEP257. Otherwise it's
> unclear
> > > whether additional spacing around the docstring should be added or not.
And
> > the
> > > docstrings look kind of messy when this is not consistent.
> > > 
> > > For myself I don't see any value in additional spaces around docstrings.
It
> > > seems like they only make less text fit into one line. One-line docstring
> > > examples in PEP257 don't have any surrounding whitespace, so I usually
just
> > > follow that style.
> > > 
> > > Does anyone have any arguments for surrounding whitespace?
> > 
> > Well I brought this up because you mentioned that i should surround them in
> > spaces on a previous review, even thoough i agree with you. So yes, we
should
> > write this down somewhere.
> 
> I'm used to add leading/trailing new lines to docstrings, because the text and
> quotes are aligned, when spanning over multiple lines:
> 
>   def foo():
>       """Subsequent lines continue in a different column
>       which makes it somewhat harder to read"""
>       pass
> 
>   def foo():
>       """
>       The first line starts in the same column
>       as subsequent lines, forming one block.
> 
>       The quotes, opening/closing the docstring use the
>       same columns as well, making it easier to identify
>       docstrings without relying on syntax highlighting.
>       """
>       pass
> 
> I wouldn't insist sticking to it though, also considering that PEP257 seems to
> suggest to omit the extra newlines in its examples. That said, if we go
without
> the extra newlines, IMO there is no need to adapt the coding practices, as we
> already refer to PEP257 there. But we might as well decide for the extra
> newlines for the reasons above, if they convince you guys.

PEP257 recommends that the summary in multiline docstrings is kept to one line.
This for the most part addresses the readability concern above. The most
elaborate multiline docstring examples there look like this:

def foo():
    """Summary line.

    Follow up text that is not aligned with the summary line but
    is separated from it by an empty line so it looks ok.
    """

Sometimes it's hard to keep the summary to one line so we can make exceptions
but in general we could follow this style. What's annoying is that while the
examples seem to suggest this style, there are other examples that deviate from
it and the text of PEP257 doesn't actually give giudance for all aspects (like
surrounding whitespace). That's why I thought that we could augment it a bit in
our style guide.

[Wladimir Palant, 2017-04-04 13:13:33]

LGTM

[Jon Sonesen, 2017-04-04 13:17:10]

On 2017/04/03 10:25:16, Vasily Kuznetsov wrote:
> https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py
> File tests/test_utils.py (right):
> 
>
https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
> tests/test_utils.py:22: """Load template from inside sitescripts."""
> On 2017/04/03 09:03:23, Jon Sonesen wrote:
> > On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> > > On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > > > Perhaps the docstring here should include the additional space before
the
> > text
> > > > inside of it?
> > > 
> > > Thanks for bringing this up. Maybe we should say something about docstring
> > > format in our style guide beyond just following PEP257. Otherwise it's
> unclear
> > > whether additional spacing around the docstring should be added or not.
And
> > the
> > > docstrings look kind of messy when this is not consistent.
> > > 
> > > For myself I don't see any value in additional spaces around docstrings.
It
> > > seems like they only make less text fit into one line. One-line docstring
> > > examples in PEP257 don't have any surrounding whitespace, so I usually
just
> > > follow that style.
> > > 
> > > Does anyone have any arguments for surrounding whitespace?
> > 
> > Well I brought this up because you mentioned that i should surround them in
> > spaces on a previous review, even thoough i agree with you. So yes, we
should
> > write this down somewhere.
> 
> Hm. Are you sure? I don't think I would suggest this. Perhaps we've
> misunderstood each other.
> 
>
https://codereview.adblockplus.org/29398791/diff/29399792/tests/test_utils.py...
> tests/test_utils.py:22: """Load template from inside sitescripts."""
> On 2017/04/03 09:51:43, Sebastian Noack wrote:
> > On 2017/04/03 09:03:23, Jon Sonesen wrote:
> > > On 2017/04/03 08:59:25, Vasily Kuznetsov wrote:
> > > > On 2017/04/03 07:42:41, Jon Sonesen wrote:
> > > > > Perhaps the docstring here should include the additional space before
> the
> > > text
> > > > > inside of it?
> > > > 
> > > > Thanks for bringing this up. Maybe we should say something about
docstring
> > > > format in our style guide beyond just following PEP257. Otherwise it's
> > unclear
> > > > whether additional spacing around the docstring should be added or not.
> And
> > > the
> > > > docstrings look kind of messy when this is not consistent.
> > > > 
> > > > For myself I don't see any value in additional spaces around docstrings.
> It
> > > > seems like they only make less text fit into one line. One-line
docstring
> > > > examples in PEP257 don't have any surrounding whitespace, so I usually
> just
> > > > follow that style.
> > > > 
> > > > Does anyone have any arguments for surrounding whitespace?
> > > 
> > > Well I brought this up because you mentioned that i should surround them
in
> > > spaces on a previous review, even thoough i agree with you. So yes, we
> should
> > > write this down somewhere.
> > 
> > I'm used to add leading/trailing new lines to docstrings, because the text
and
> > quotes are aligned, when spanning over multiple lines:
> > 
> >   def foo():
> >       """Subsequent lines continue in a different column
> >       which makes it somewhat harder to read"""
> >       pass
> > 
> >   def foo():
> >       """
> >       The first line starts in the same column
> >       as subsequent lines, forming one block.
> > 
> >       The quotes, opening/closing the docstring use the
> >       same columns as well, making it easier to identify
> >       docstrings without relying on syntax highlighting.
> >       """
> >       pass
> > 
> > I wouldn't insist sticking to it though, also considering that PEP257 seems
to
> > suggest to omit the extra newlines in its examples. That said, if we go
> without
> > the extra newlines, IMO there is no need to adapt the coding practices, as
we
> > already refer to PEP257 there. But we might as well decide for the extra
> > newlines for the reasons above, if they convince you guys.
> 
> PEP257 recommends that the summary in multiline docstrings is kept to one
line.
> This for the most part addresses the readability concern above. The most
> elaborate multiline docstring examples there look like this:
> 
> def foo():
>     """Summary line.
> 
>     Follow up text that is not aligned with the summary line but
>     is separated from it by an empty line so it looks ok.
>     """
> 
> Sometimes it's hard to keep the summary to one line so we can make exceptions
> but in general we could follow this style. What's annoying is that while the
> examples seem to suggest this style, there are other examples that deviate
from
> it and the text of PEP257 doesn't actually give giudance for all aspects (like
> surrounding whitespace). That's why I thought that we could augment it a bit
in
> our style guide.

Yeah I think you are right here. I am mis-remembering so thanks for clearing
that up. 

LGTM

[Vasily Kuznetsov, 2017-04-04 16:57:12]

Thanks to everyone for the review. I will check sitescripts configs on the
servers to make sure this doesn't cause unexpected regressions and then will
push it.