Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes for Django 5.0. #1606

Closed
wants to merge 3 commits into from
Closed

Fixes for Django 5.0. #1606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d03ba6e
Add django.setup() call to runtests.py.
carltongibson Aug 31, 2023
0b42447
Initial pass at 5.0 compat.
carltongibson Sep 1, 2023
f279fb7
Re-require passing tests against Django main.
carltongibson Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 33 additions & 7 deletions django_filters/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@
RangeWidget,
)

# Compat for Django >= 5.0
try:
from django.utils.choices import normalize_choices
except ImportError:
normalize_choices = None

DJANGO_50 = normalize_choices is not None

Comment on lines +20 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified:

Suggested change
# Compat for Django >= 5.0
try:
from django.utils.choices import normalize_choices
except ImportError:
normalize_choices = None
DJANGO_50 = normalize_choices is not None
try:
from django.utils.choices import normalize_choices
except ImportError:
DJANGO_50 = False
else:
DJANGO_50 = True


class RangeField(forms.MultiValueField):
widget = RangeWidget
Expand Down Expand Up @@ -257,22 +265,32 @@ def __init__(self, *args, **kwargs):

super().__init__(*args, **kwargs)

def _get_choices(self):
return super()._get_choices()
if not DJANGO_50:

def _get_choices(self):
return super()._get_choices()

def _set_choices(self, value):
super()._set_choices(value)
value = self.iterator(self, self._choices)
def _set_choices(self, value):
super()._set_choices(value)
value = self.iterator(self, self._choices)

self._choices = self.widget.choices = value
self._choices = self.widget.choices = value

choices = property(_get_choices, _set_choices)
choices = property(_get_choices, _set_choices)


# Unlike their Model* counterparts, forms.ChoiceField and forms.MultipleChoiceField do not set empty_label
class ChoiceField(ChoiceIteratorMixin, forms.ChoiceField):
iterator = ChoiceIterator

if DJANGO_50:

@forms.ChoiceField.choices.setter
def choices(self, value):
self._choices = self.widget.choices = self.iterator(
self, normalize_choices(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few interesting things here:

  1. Ideally we'd not rehash the Django internals here, i.e. self._choices = self.widget.choices = ...
  2. For equivalent behaviour as in Django 5.0 it ought to be ... = normalize_choices(self.iterator(...))
  3. To make the normalization lazy, ChoiceIterator should extend django.utils.choices.ChoiceIterator
    • Maybe I should have called that django.utils.choices.BaseChoiceIterator? 🤔
  4. We should push the normalize_choices() call into ChoiceIterator.__iter__()
  5. It's a shame that we have to implement this twice rather than once in the mixin

I think all of this can be solved. Will open an alternate PR soon...

)

def __init__(self, *args, **kwargs):
self.empty_label = kwargs.pop("empty_label", settings.EMPTY_CHOICE_LABEL)
super().__init__(*args, **kwargs)
Expand All @@ -281,6 +299,14 @@ def __init__(self, *args, **kwargs):
class MultipleChoiceField(ChoiceIteratorMixin, forms.MultipleChoiceField):
iterator = ChoiceIterator

if DJANGO_50:

@forms.MultipleChoiceField.choices.setter
def choices(self, value):
self._choices = self.widget.choices = self.iterator(
self, normalize_choices(value)
)

def __init__(self, *args, **kwargs):
self.empty_label = None
super().__init__(*args, **kwargs)
Expand Down
2 changes: 2 additions & 0 deletions runtests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
import os
import sys

import django
from django.core.management import execute_from_command_line


def runtests():
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "tests.settings")
django.setup()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was due to a deep circular import issue that didn't flag up when running Django's test suite.

See django/django#17215 for where this was fixed.

argv = sys.argv[:1] + ["test"] + sys.argv[1:]
execute_from_command_line(argv)

Expand Down
3 changes: 0 additions & 3 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,3 @@ commands = python -Werror ./runtests.py --testrunner xmlrunner.extra.djangotestr
deps =
{[latest]deps}
-rrequirements/test-ci.txt

[testenv:{py310, py311}-latest]
ignore_outcome = True
Loading