-
Notifications
You must be signed in to change notification settings - Fork 769
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.
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
Fixes for Django 5.0. #1606
Fixes for Django 5.0. #1606
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
+ Coverage 98.58% 98.59% +0.01%
==========================================
Files 15 15
Lines 1271 1285 +14
==========================================
+ Hits 1253 1267 +14
Misses 18 18
|
b035bf2
to
0b42447
Compare
Hey @ngnpope 👋 Don't suppose I could ask for your opinion here could I please? 🎁 |
This only comes up in tox. (Grrr.) Calling |
Will take a look... 👀 |
from django.core.management import execute_from_command_line | ||
|
||
|
||
def runtests(): | ||
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "tests.settings") | ||
django.setup() |
There was a problem hiding this comment.
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.
# 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 | ||
|
There was a problem hiding this comment.
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:
# 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 |
@forms.ChoiceField.choices.setter | ||
def choices(self, value): | ||
self._choices = self.widget.choices = self.iterator( | ||
self, normalize_choices(value) |
There was a problem hiding this comment.
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:
- Ideally we'd not rehash the Django internals here, i.e.
self._choices = self.widget.choices = ...
- For equivalent behaviour as in Django 5.0 it ought to be
... = normalize_choices(self.iterator(...))
- To make the normalization lazy,
ChoiceIterator
should extenddjango.utils.choices.ChoiceIterator
- Maybe I should have called that
django.utils.choices.BaseChoiceIterator
? 🤔
- Maybe I should have called that
- We should push the
normalize_choices()
call intoChoiceIterator.__iter__()
- 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...
See #1607 for an alternative 🙂 |
Thanks @ngnpope 🦸♀️ Looks good. I'll give it a proper read over the weekend. 🎁 |
Closing in favour of #1607 |
Refs #1605.
b035bf2 is needed to surface the ChoiceIterator issue, otherwise we hit apps not loaded setting up the test run.
(@felixxm don't suppose you know when this snuck in do you? Ta! 😊)