search

niwinz / django-jinja

Simple and nonobstructive jinja2 integration with Django.
http://niwinz.github.io/django-jinja/latest/
BSD 3-Clause "New" or "Revised" License
363 stars 102 forks source link

Instrumented rendering removed in 2e1aefd4fa61272881c8c91e3c4164cc6b2cd1ca #149

Closed ghost closed 8 years ago

ghost commented 8 years ago

Hi @niwinz, I was wondering why the hook to setup_test_environment was removed in 2e1aefd4fa61272881c8c91e3c4164cc6b2cd1ca, wouldn't that mean that it's currently not working on 1.7? Or would this be covered by 7b605259c1803dfe715cd721656d8e82d418daee on both 1.7 and 1.8?

jezdez commented 8 years ago

Woops, written from the wrong account, that was me.

niwinz commented 8 years ago

The template instrumentation is already covered by this code: https://github.com/niwinz/django-jinja/blob/2e1aefd4fa61272881c8c91e3c4164cc6b2cd1ca/django_jinja/base.py#L75 so I think that monky patching is just duplicated and not necessary. All test passes related to instrumentation. But If I'm wrong, let me know.

Later, the current master will be the last version that supports django <= 1.7 (I will release a 1.5 probably sooner). And the next version 2.0.0 removes all support for django <= 1.7. This approach is taken because the code was grow unnecessary with the corresponding complexity (because of support django <= 1.6, django 1.7 and django >= 1.8 (pre apps, apps and pluggable templates)).

jezdez commented 8 years ago

@niwinz Ah, thank you! That plan makes total sense, the dual implementation must be painful to maintain. Many thanks for your work in that regard.

jezdez commented 8 years ago

Ah a related question, we're using the last released version of django-jinja in an attempt to upgrade to 1.8 and found an issue in which an app was passing in a Django Context object to the template render method.

Since the old 1.7 code has a native Jinja2 Template class that collapses the Context object into a dict (via dict_from_context) I was able to fix that by assigning that custom template class to the Jinja2 environment of the 1.8 backend (https://github.com/mozilla/kuma/commit/ecb253b0d39f667e7963c75010813ca4f61499ae). Should that collapsing also be happening in django-jinja's 1.8 backend?

niwinz commented 8 years ago

I'm not sure if I'm understand your question. The django-jinja 1.8 backend works a little bit different to the <= 1.7 related code. But as I can observe in the code, the collapsing is not happening on the 1.8 backend and your fix should work in the same way.

However, it maybe should be fixed directly in django-jinja and avoid do any additional fix on the user code base.

niwinz commented 8 years ago

2.0.0 is released already with fixed instrumentation.