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.

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

Support Django 5.0 #438

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Support Django 5.0 #438

merged 2 commits into from
Mar 27, 2024

Conversation

adamchainz
Copy link
Contributor

Builds on #424, which should be merged first. Add support for Django 5.0.

@adamchainz
Copy link
Contributor Author

Currently there are three test failures:

FAILED django_countries/tests/test_admin_filters.py::TestCountryFilter::test_choices - AssertionError: False != True
FAILED django_countries/tests/test_fields.py::TestModelForm::test_translated_choices - AssertionError: 'Afghanistan' != 'Afganio'
FAILED django_countries/tests/test_widgets.py::TestCountrySelectWidget::test_render - AssertionError: 0 != 1 : Found 0 instances of '<option value="AU">

The latter two cases are caused by Django’s changes to handling choices. It seems that part of this change means that Django iterates over the Countries class at field initialization, rather than lazily when required. This means that the translations are done once rather than on-demand.

One option is to instead use a no-argument callable, which will always be called later. I tried adding __call__ to Countries but that didn’t work, I suspect a wrapping function will be required.

@adamchainz
Copy link
Contributor Author

I think we can stop using LazyTypedChoiceField / LazyTypedMultipleChoiceField and all related classes on Django 5.0, since a callable for choices is treated lazily by Django. But unfortunately there’s a bug with that laziness, which I’ve reported upstream: https://code.djangoproject.com/ticket/34899 .

@adamchainz
Copy link
Contributor Author

The fix ( django/django#17370 ) was merged and released in Django 5.0b1. I have updated the code to use the new callable choices support and made the Lazy* classes no-ops on Django 5.0+.

I also added a fix for the admin change:

The django.contrib.admin.AllValuesFieldListFilter, ChoicesFieldListFilter, RelatedFieldListFilter, and RelatedOnlyFieldListFilter admin filters now handle multi-valued query parameters.

Everything is passing now.

@adamchainz adamchainz force-pushed the django_5.0 branch 3 times, most recently from 91efae7 to b01ed85 Compare November 8, 2023 14:24
@washeck
Copy link

washeck commented Nov 28, 2023

@SmileyChris can this please be merged and released? It is blocking upgrade to Django 5.0 (I am testing RC1 now). Thank you.

@adamchainz
Copy link
Contributor Author

You can install my commit from the Git repo with this specification for Pip:

django-countries @ git+https://github.com/SmileyChris/django-countries@58f258402072a18756a15daa81325428382bd946

(or with the appropriate syntax for Poetry etc.)

That’s what I’ve done for testing my client’s project on Django 5.0 RC1.

@washeck
Copy link

washeck commented Nov 28, 2023

Yes, that's what I'm doing (django_5.0 branch from your fork) and it's working so I think it would be good to issue the release before Django 5.0 is released and people start upgrading. Thanks you for the PR.

@@ -36,10 +36,10 @@ deps =
pyuca,legacy: pyuca
legacy: Django==3.2.*
previous: Django==4.0.*
latest: Django==4.1.*
latest: Django~=5.0b1
Copy link

Choose a reason for hiding this comment

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

I guess this could be updated. Then @SmileyChris, please push this or give someone the commit bit so the project is not blocked when you are out of time!

@kribot
Copy link

kribot commented Dec 16, 2023

I did install from pip using the above command, but I still have the below issue in django 5. Am I missing something ? @adamchainz would you have any idea ? It seems we are quite a few to have this issue.

\django_countries\fields.py", line 22, in <module>
    _entry_points = importlib.metadata.entry_points().get(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'EntryPoints' object has no attribute 'get'

@@ -22,8 +23,12 @@ def choices(self, changelist):
"display": _("All"),
}
for lookup, title in self.lookup_choices(changelist):
if django.VERSION >= (5, 0):
selected = force_str(lookup) in value
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to change this line so it works when no country is selected in the CountryFilter list_filter I'm using, otherwise it'll throw a TypeError exception that argument of type 'NoneType' is not iterable

Suggested change
selected = force_str(lookup) in value
selected = value and force_str(lookup) in value

Copy link
Collaborator

@MarcoGlauser MarcoGlauser Mar 25, 2024

Choose a reason for hiding this comment

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

@adamchainz any objection to this?

@zyv
Copy link

zyv commented Dec 25, 2023

Any new on when this is going to be released? Django 5 it out for a while and after I tried to upgrade, my admin pages got broken. Here is a corresponding Django ticket:

https://code.djangoproject.com/ticket/35046

zyv added a commit to zyv/pilot_log that referenced this pull request Dec 25, 2023
@KlemenS189
Copy link

Any chance this could be released? It's blocking our upgrade to Django 5.0.
Thanks.

@JulianTenschert
Copy link
Contributor

JulianTenschert commented Jan 8, 2024

I did install from pip using the above command, but I still have the below issue in django 5. Am I missing something ? @adamchainz would you have any idea ? It seems we are quite a few to have this issue.

\django_countries\fields.py", line 22, in <module>
    _entry_points = importlib.metadata.entry_points().get(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'EntryPoints' object has no attribute 'get'

It looks like @adamchainz' code works with Python < 3.10 perfectly. However, the code doesn't work with Python 3.12.

If I understand correctly, the importlib.metadata API has been changed since Python 3.10.

Other projects had the same issue, e.g. ewels/rich-click#140.

I've modified Adam's code to make it work with Python 3.10 and above: JulianTenschert@f085ab1. This works well.

@adamchainz adamchainz mentioned this pull request Jan 24, 2024
@andriijas
Copy link

@SmileyChris Whats needed to move forward with this?
How can we help?

Thanks

co-authored-by: Julian Tenschert <[email protected]>
@adamchainz
Copy link
Contributor Author

I just squashed @JulianTenschert ’s commit into mine to make merging or installing from git easier.

@claudep
Copy link

claudep commented Mar 23, 2024

Thanks for your efforts, however I think the current situation is not sustainable in the longer term. If anyone has direct contacts with Chris personally or someone else at Lincoln Loop, it would be very nice to push a bit so as to unblock this package and put it in a co-maintainership situation (by Jazzband adoption or any other mean).

@MarcoGlauser
Copy link
Collaborator

MarcoGlauser commented Mar 25, 2024

Hey everyone,
I managed to get in contact with SmileyChris and he gave me contributor access to the repo. I'm currently trying to familiarize myself with the state of the project.
@adamchainz Thank you for creating this PR and keeping it alive. What is the current state of the PR?
Does it still rely on the Django 4.2 PR or have they been combined in the meantime?
Does it also address the python 3.12 fixes?

From my side, this PR takes priority and after I will look into updating the test matrix to test all currently supported django and python combination instead of only the latest.

@adamchainz
Copy link
Contributor Author

adamchainz commented Mar 25, 2024

I did not combine the Django 4.2 PR into this one, but I did merge in the Python 3.12 fix. Thank you for taking over the project!

@SmileyChris SmileyChris merged commit 4d3d7b5 into SmileyChris:main Mar 27, 2024
4 checks passed
@SmileyChris
Copy link
Owner

Sorry I've been so absent! I finally got things polished up -- I had to fix transifex not pulling in changes due to their api changes, and some tox updates for 4 but it's done, and I pushed 7.6!

@foutrelis
Copy link
Contributor

One small follow-up fix is needed: #460

Without this check even a trivial use of CountryFilter will raise an exception:

  File "/usr/local/lib/python3.12/site-packages/django_countries/filters.py", line 27, in choices
    selected = force_str(lookup) in value
               ^^^^^^^^^^^^^^^^^^^^^^^^^^

Exception Type: TypeError at /admin/country_test/person/
Exception Value: argument of type 'NoneType' is not iterable

@adamchainz adamchainz deleted the django_5.0 branch June 29, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.