Add Jinja2 support

[HiPhish]

Resolves #165. I have added the Jinja equivalent of the django_simple_nav template. In Jinja we can make a regular Python function available as a global variable in the Jinja environment, so the code is much simpler. The user will have to add the function to his own global environment when configuring Jinja in his project. The new function can be called like this: {{ django_simple_nav('my_app.navigation.SiteNav', 'my_app/main-nav.html.j2' }}.

I have some open questions though:

• Jinja support is optional, I don't think it's appropriate to force users to download Jinja into their application if they don't want to use it. Django does not include Jinja by default either. Should we keep Jinja optional or include it by default?
• Currently the function is stored under django_simple_nav.jinja2.django_simple_nav. This looks too deeply nested to me, how about django_simple_nav.jinja2 instead?
• The current test only tries to render the template, but it does not do anything to verify the rendered text. What would be the proper way of testing this?

[HiPhish]

bump

[joshuadavidthomas]

• Jinja support is optional, I don't think it's appropriate to force users to download Jinja into their application if they don't want to use it. Django does not include Jinja by default either. Should we keep Jinja optional or include it by default?

Hm not sure where I land on this. Though I do know I don't like including it as default, as it should be opt-in, so I agree with you there. I don't mind including it as the extra and having user's be able to install via django-simple-nav[jinja]. But also, if someone's going to use jinja I'm going to assume they already have it installed and in that case should this library concerned with that? I lean towards no.

Then again, we'll need to specify jinja as a dep anyway for the tests to run, so maybe there's no harm in just putting it as an extra as you've done.

• Currently the function is stored under django_simple_nav.jinja2.django_simple_nav. This looks too deeply nested to me, how about django_simple_nav.jinja2 instead?

The nested nature of it doesn't bother me, but the fact that there's nothing else in that module other than that file does. 😄 I'm okay with top-level file at src/django_simple_nav/jinja2.py

• The current test only tries to render the template, but it does not do anything to verify the rendered text. What would be the proper way of testing this?

Honestly, probably not much more than that? You could copy the approach of the existing Django template tag tests here

django-simple-nav/tests/test_templatetags.py

Lines 18 to 90 in 4fa25a2

	def test_django_simple_nav_templatetag(req):
	template = Template(
	"{% load django_simple_nav %} {% django_simple_nav 'tests.navs.DummyNav' %}"
	)
	req.user = AnonymousUser()
	rendered_template = template.render(Context({"request": req}))
	assert count_anchors(rendered_template) == 7
	def test_templatetag_with_template_name(req):
	template = Template(
	"{% load django_simple_nav %} {% django_simple_nav 'tests.navs.DummyNav' 'tests/alternate.html' %}"
	)
	req.user = AnonymousUser()
	rendered_template = template.render(Context({"request": req}))
	assert "This is an alternate template." in rendered_template
	def test_templatetag_with_nav_instance(req):
	class PlainviewNav(DummyNav):
	items = [
	NavItem(title="I drink your milkshake!", url="/milkshake/"),
	]
	template = Template("{% load django_simple_nav %} {% django_simple_nav new_nav %}")
	req.user = baker.make(get_user_model(), first_name="Daniel", last_name="Plainview")
	rendered_template = template.render(
	Context({"request": req, "new_nav": PlainviewNav()})
	)
	assert "I drink your milkshake!" in rendered_template
	def test_templatetag_with_nav_instance_and_template_name(req):
	class DeadParrotNav(DummyNav):
	items = [
	NavItem(title="He's pinin' for the fjords!", url="/notlob/"),
	]
	template = Template(
	"{% load django_simple_nav %} {% django_simple_nav new_nav 'tests/alternate.html' %}"
	)
	req.user = baker.make(get_user_model(), first_name="Norwegian", last_name="Blue")
	rendered_template = template.render(
	Context({"request": req, "new_nav": DeadParrotNav()})
	)
	assert "He's pinin' for the fjords!" in rendered_template
	assert "This is an alternate template." in rendered_template
	def test_templatetag_with_template_name_on_nav_instance(req):
	class PinkmanNav(DummyNav):
	template_name = "tests/alternate.html"
	items = [
	NavItem(title="Yeah Mr. White! Yeah science!", url="/science/"),
	]
	template = Template("{% load django_simple_nav %} {% django_simple_nav new_nav %}")
	req.user = baker.make(get_user_model(), first_name="Jesse", last_name="Pinkman")
	rendered_template = template.render(
	Context({"request": req, "new_nav": PinkmanNav()})
	)
	assert "Yeah Mr. White! Yeah science!" in rendered_template
	assert "This is an alternate template." in rendered_template

and just render the template and count the anchors? A silly test, but it proves the template renders without error and renders correctly.

[joshuadavidthomas]

LOL, but definitely change the name, if nothing else.

[joshuadavidthomas]

This looks good to me, but I've only ever been a consumer of Jinja templates through other means -- I'm ignorant when it comes to custom tags over there.

[HiPhish]

I have now added tests which mirror test_templatetags. I think that's enough testing, we are covering the same functionality as the Django template tag, nothing more and nothing less. I would like a review now.

There is one unfortunate name collision I encountered: a nav item has the field items, but it is also a dict which has the method items. In Jinja attributes take precedence over dict entries, so item.items refers to the method, not item['items']. This tripped me up in the template:

{% if item.items %}
  <ul>
    {{ loop(item.items) }}
  </ul>
{% endif %}

This would try to iterate over the items method, which makes no sense. Instead the snippet needs to be written as follows:

{% if item['items'] %}
  <ul>
    {{ loop(item['items']) }}
  </ul>
{% endif %}

I expect this will trip up quite a few Jinja users. How about renaming items to children? We could keep both items and children in the dict for backwards compatibility for the time being.

[HiPhish]

I have moved the implementation file to a more reasonable location and added documentation for Jinja. I am hereby done and wish to request a full review.

[HiPhish]

bump Any else left to do?

[joshuadavidthomas]

@HiPhish Ah sorry, time got away from me. Do you mind adding yourself to the AUTHORS.md file? I'd like to capture your contribution.

[HiPhish]

Done. I didn't think something this small warrants being added, but here you go.

[joshuadavidthomas]

Done. I didn't think something this small warrants being added, but here you go.

Well, I dunno, adding support for another templating engine is no small thing! Even if the diff ends up being small, still a pretty big feature addition.

Thanks again!