search

carltongibson / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.io/en/main/
Other
4.42k stars 766 forks source link

0.15.1 crashes if imported #506

Closed yardensachs closed 7 years ago

yardensachs commented 7 years ago

Because of this: https://github.com/carltongibson/django-filter/blob/master/django_filters/rest_framework/backends.py#L37

Django looks for a DjangoTemplates engine in the settings, and I don't use Django templates, I use Jinja2.

  File "/home/ubuntu/virtualenvs/venv-3.5.1/lib/python3.5/site-packages/rest_framework_filters/__init__.py", line 3, in <module>
    from .filterset import FilterSet
  File "/home/ubuntu/virtualenvs/venv-3.5.1/lib/python3.5/site-packages/rest_framework_filters/filterset.py", line 11, in <module>
    from django_filters import filterset, rest_framework
  File "/home/ubuntu/virtualenvs/venv-3.5.1/lib/python3.5/site-packages/django_filters/rest_framework/__init__.py", line 3, in <module>
    from .backends import DjangoFilterBackend
  File "/home/ubuntu/virtualenvs/venv-3.5.1/lib/python3.5/site-packages/django_filters/rest_framework/backends.py", line 37, in <module>
    template_default = Template(FILTER_TEMPLATE)
  File "/home/ubuntu/virtualenvs/venv-3.5.1/lib/python3.5/site-packages/django/template/base.py", line 184, in __init__
    engine = Engine.get_default()
  File "/home/ubuntu/virtualenvs/venv-3.5.1/lib/python3.5/site-packages/django/template/engine.py", line 81, in get_default
    "No DjangoTemplates backend is configured.")
django.core.exceptions.ImproperlyConfigured: No DjangoTemplates backend is configured.

Thank god for tests :)

carltongibson commented 7 years ago

Hmmm. OK. A failing test case would be a great help here.

My first thought — which may be wrong — is that you could just add the Django template engine: you're allowed multiples right?

This would be necessary to use DRF's Browsable API etc anyway. (For which you'll now tell me you're not using DRF, or not using the Browsable API?)

yardensachs commented 7 years ago

You are correct to assume that I am not using the Browsable API, and adding DajngoTemlpates to the settings does fix this issue.

But I guess the decision whether or not to be decoupled from Django templates needs to be made. and if not, add that to the docs and testing.

carltongibson commented 7 years ago

OK. First: 👍 You've got a workaround.

Second:

But I guess the decision whether or not to be decoupled from Django templates needs to be made. and if not, add that to the docs and testing.

Entirely. I'm just looking at the template_default definition. It'd probably be enough here just to defer that to runtime. (By the time it's needed you'd better have the Django template engine configured...)

yardensachs commented 7 years ago

Sure, the best first thing that can fix the i-broke-the-internet - is moving it to runtime :)

You might want to have this become a new minor release and not a patch release. Because of how this is kind of a backwards incompatible change (because I had to change something in my code to upgrade).

Not sure if this issue is this package's fault. I did not choose to upgrade manually, pip did that all by it self because of this line: https://github.com/philipn/django-rest-framework-filters/blob/master/setup.py#L46 django-filter>=0.15.0

rpkilby commented 7 years ago

Entirely. I'm just looking at the template_default definition. It'd probably be enough here just to defer that to runtime. (By the time it's needed you'd better have the Django template engine configured...)

Yep - there's not much of a reason to do this at the module level. Template parsing is fast and regardless - the default templates are tiny. I don't think there would be any significant overhead to moving the Template() call to the to_html method.

You might want to have this become a new minor release and not a patch release

This is essentially a bug fix - a patch release should be fine? The only change would be where the template is instantiated.

carltongibson commented 7 years ago
  1. I doubt we broke the internet. :-)
  2. patch release is fine. SemVer is a tool, not a law of nature.
rpkilby commented 7 years ago

I'm working on the test case/patch btw