Issue 5044 - Support absolute paths for Jinja templates

tests/test_utils.py

Index: tests/test_utils.py
===================================================================
new file mode 100644
--- /dev/null
+++ b/tests/test_utils.py
@@ -0,0 +1,38 @@
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2006-2017 eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import pytest
+
+from sitescripts.utils import get_template
+
+
+def test_get_template_default_path():
+    """Load template from inside sitescripts."""

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

Perhaps the docstring here should include the additional space before the text
inside of it?

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

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?

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

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.

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

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:15]

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

[Vasily Kuznetsov, 2017/04/03 10:25:15]

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.

+    template = get_template('__init__.py')
+    assert template.render({}).startswith('# This file')
+
+
+@pytest.mark.parametrize('mode', ['relative', 'absolute'])
+def test_get_template(tmpdir, mode):
+    """Load template using relative or absolute path."""

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

Same here?

+    template_path = tmpdir.join('template.tmpl')
+    template_path.write('value = {{ value }}')
+
+    if mode == 'absolute':
+        template = get_template(template_path.strpath)
+    else:
+        template = get_template('template.tmpl', template_path=tmpdir.strpath)

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

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]

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]

I see, thanks

+
+    assert template.render({'value': 1}) == 'value = 1'