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

Fix CallableChoiceIterator import on Django 5.0+ #10846

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

laymonage
Copy link
Member

This was changed in django/django#16943. It's not enough to make our tests pass against Django's main branch though, as some fixes need to be done in the upstream django-filter package.

@squash-labs
Copy link

squash-labs bot commented Aug 31, 2023

Manage this branch in Squash

Test this branch here: https://laymonagefix-import-django5-2kuq9.squash.io

@carltongibson
Copy link
Contributor

Hey @laymonage — I just spotted this too.

  File "/Users/carlton/Projects/Django/django-filter/.tox/py311-latest/lib/python3.11/site-packages/django/forms/fields.py", line 874, in __init__
    self.choices = choices
    ^^^^^^^^^^^^
  File "/Users/carlton/Projects/Django/django-filter/django_filters/fields.py", line 264, in _set_choices
    super()._set_choices(value)
    ^^^^^^^^^^^^^^^^^^^^
AttributeError: 'super' object has no attribute '_set_choices'

I'm working towards a release there, so if you want to open a Issue/PR, with what you know that would be 🥰

@laymonage
Copy link
Member Author

Hey @carltongibson, good to see you here! I suspect the issue is around the refactoring of the _get_choices and _set_choices method into proper Python @property getter and setter. I briefly looked at django-filter code when I wrote this PR but it looks like it might need some clever handling of the choices property if you want to support both Django pre-5.0 and 5.0+. I can try looking into it on the weekend, but until then it's all yours 😉

@carltongibson
Copy link
Contributor

Thanks @laymonagecarltongibson/django-filter#1605 is the tracker for this my end. 👍

@carltongibson
Copy link
Contributor

Oh, @laymonage, If you could comment your support matrix that would be handy... (I'll do a release here, but I'm looking to drop PY37 and Django <4.2... and I don't want to cause you issues.) Ta

@laymonage
Copy link
Member Author

laymonage commented Aug 31, 2023

@carltongibson We've dropped PY37 in #10676. We're supporting Django versions that are not EOL (see https://docs.wagtail.org/en/stable/releases/upgrading.html#compatible-django-python-versions for details). Latest feature release is 5.1, with the main branch on 5.2.

Given that Django 5.0 is not until December and our next feature release is in November, it's likely we won't add official support for Django 5.0 until February next year. See our release schedule for details: https://github.com/wagtail/wagtail/wiki/Release-schedule

That said, we'll still support Django 3.2 even when 4.1 goes EOL. We're currently capping our django-filter dependency in this way:

"django-filter>=2.2,<24",

If the next release of django-filter bumps Django requirement to 4.2 I think it's fine? Not sure how it's going to work but pip will probably be smart enough to use the last version that still supported the Django version in use by our users.

For new projects, our project template defaults to >=4.2:

@carltongibson
Copy link
Contributor

@laymonage Preliminary compat here:

A bit of tidying, and a release shortly.

@zerolab
Copy link
Contributor

zerolab commented Sep 18, 2023

@laymonage https://pypi.org/project/django-filter/23.3/ is out so this can proceed

@laymonage laymonage force-pushed the fix-import-django5 branch 2 times, most recently from 10affcd to c49518c Compare September 18, 2023 08:25
@laymonage laymonage marked this pull request as ready for review September 18, 2023 08:25
@laymonage laymonage requested a review from zerolab September 18, 2023 08:50
Copy link
Contributor

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Looks good.

@laymonage laymonage merged commit 4a09746 into wagtail:main Sep 18, 2023
12 checks passed
@laymonage
Copy link
Member Author

Thank you for your great work @carltongibson! 🌟

@laymonage laymonage self-assigned this Sep 18, 2023
@laymonage laymonage added this to the 5.2 milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants