Added support for Django 5.0. #1607
Conversation
ngnpope
force-pushed
the
fixes-for-django-5.0-alternate
branch
from
b24047d
to
b62e99b
Compare
django_filters/fields.py
Outdated
| @@ -210,7 +219,7 @@ def clean(self, value): | |||
| return value | |||
|
|
|||
|
|
|||
| class ChoiceIterator: | |||
| class ChoiceIterator(BaseChoiceIterator): | |||
There was a problem hiding this comment.
In Django 5.0 I added django.utils.choices.ChoiceIterator as a base class for the following:
django.forms.models.ModelChoiceIteratordjango.utils.choices.CallableChoiceIterator(ordjango.forms.fields.CallableChoiceIteratorin Django < 5.0)
It is expected that a subclass of this will handle normalization via use of django.utils.choices.normalize_choices() or manually (as is the case for ModelChoiceIterator which yields 2-tuples).
| super()._set_choices(value) | ||
| value = self.iterator(self, self._choices) |
There was a problem hiding this comment.
This double setting case is required in Django < 5.0 because callables are handled very specifically whereas it's all handled in django.utils.choices.normalize_choices() for Django 5.0.
| # Simple `super()` syntax for calling a parent property setter is | ||
| # unsupported. See https://github.com/python/cpython/issues/59170 | ||
| super(ChoiceIteratorMixin, self.__class__).choices.__set__(self, value) |
There was a problem hiding this comment.
This is the most unfortunate bit, but works well nonetheless.
Ideally this whole override ought to be as simple as super().choices = self.iterator(self, value).
But currently there is no support for calling the setter of a parent property, see python/cpython#59170, although there does seem to be indication that a fix for this would be acceptable, see python/cpython#29950 (comment).
There is this workaround which is even used in Python itself but there were challenges observed regarding mixins. I have managed to make this work for this mixin by using super(ChoiceIteratorMixin, self.__class__) instead of super(self.__class__, self.__class__) which ensures that when calling e.g. ChoiceField().choices = (for a subclass of the mixin), the super() call will not be passed ChoiceField as the value for self.__class__ as the first argument. Passing ChoiceIteratorMixin means that the next lookup in the MRO will be for forms.ChoiceField.choices.
There was a problem hiding this comment.
(I should also mention that this only works if we're using @choices.setter here and the property is defined here, not in the parent otherwise we end up with infinite recursion. Another reason why the getter must be redefined here.)
There was a problem hiding this comment.
I don't think I have enough expertise to review this (especially with the nuances around the property). What I had in mind was something like #1606, but Nick's solution here seems very nice. Looking at the CI it seems to work, so I suppose that's all there is for now? Happy to help with anything else though @carltongibson 🙌
Some third-party applications, e.g. `django-filter`, already define their own `ChoiceIterator`, so renaming this `BaseChoiceIterator` will be a better fit and avoid any potential confusion. See carltongibson/django-filter#1607.
django_filters/fields.py
Outdated
| @@ -17,6 +17,15 @@ | |||
| RangeWidget, | |||
| ) | |||
|
|
|||
| try: | |||
| from django.utils.choices import ChoiceIterator as BaseChoiceIterator | |||
There was a problem hiding this comment.
@carltongibson A heads up that I've opened django/django#17223 to rename ChoiceIterator to BaseChoiceIterator. Will update this PR to simplify this once that's been merged.
There was a problem hiding this comment.
Great. Thanks Nick. I'll hold off here for that.
Some third-party applications, e.g. `django-filter`, already define their own `ChoiceIterator`, so renaming this `BaseChoiceIterator` will be a better fit and avoid any potential confusion. See carltongibson/django-filter#1607.
ngnpope
force-pushed
the
fixes-for-django-5.0-alternate
branch
from
b62e99b
to
8306c8f
Compare
Codecov Report
@@ Coverage Diff @@
## main #1607 +/- ##
==========================================
+ Coverage 98.58% 98.59% +0.01%
==========================================
Files 15 15
Lines 1271 1283 +12
==========================================
+ Hits 1253 1265 +12
Misses 18 18
|
Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
ngnpope
force-pushed
the
fixes-for-django-5.0-alternate
branch
from
8306c8f
to
117f364
Compare
|
Ok. Updated now the change for |
Refs #1605.
Alternative to #1606.