-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Update checks.py #10462
Update checks.py #10462
Conversation
Manage this branch in SquashTest this branch here: https://phijma-leukeleupatch-1-ihp5w.squash.io |
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.
@phijma-leukeleu thanks for this, could you please update this code to check the Django version instead. This is the way we resolve compatibility issues and also ensure that we can find when to remove the legacy code once we drop support for Django versions in the future.
Examples
https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/modeladmin/views.py#L347
https://github.com/wagtail/wagtail/blob/main/wagtail/images/migrations/0025_alter_image_file_alter_rendition_file.py#L24
if DJANGO_VERSION >= (4, 2):
# ...
Prevent "RemovedInDjango51Warning: The DEFAULT_FILE_STORAGE setting is deprecated. Use STORAGES instead."
@lb- I looked at it, but not sure how to do this in as few lines as possible. This is all I can think of: if DJANGO_VERSION >= (4, 2):
try:
file_storage = getattr(settings, "STORAGES")["default"]["BACKEND"]
except AttributeError:
file_storage = getattr(settings, "DEFAULT_FILE_STORAGE", None)
elif DJANGO_VERSION >= (5, 1):
file_storage = getattr(settings, "STORAGES")["default"]["BACKEND"]
else:
file_storage = getattr(settings, "DEFAULT_FILE_STORAGE", None) Because in django 4.2, both are still valid. |
Sorry for the delay, let's go with that @phijma-leukeleu - it's a bit verbose I agree but makes it easier to understand the version based context. |
Hey @phijma-leukeleu let us know if you are still able to make those changes based on the initial round of feedback. |
Committed the suggested change above. |
Just started the CI, added needs review flag. Thanks @phijma-leukeleu We are getting a
|
Co-authored-by: LB (Ben Johnston) <mail@lb.ee>
wagtail/admin/checks.py
Outdated
file_storage = getattr(settings, "STORAGES")["default"]["BACKEND"] | ||
except AttributeError: | ||
file_storage = getattr(settings, "DEFAULT_FILE_STORAGE", None) | ||
elif DJANGO_VERSION >= (5, 1): |
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.
This elif
doesn't make sense here - if DJANGO_VERSION is greater than (5,1) then the if DJANGO_VERSION >= (4, 2):
will have been taken, so this code is never reached.
wagtail/admin/checks.py
Outdated
file_storage = getattr(settings, "DEFAULT_FILE_STORAGE", None) | ||
from django import VERSION as DJANGO_VERSION | ||
|
||
if DJANGO_VERSION >= (4, 2): |
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.
🤦
@gasman Something like this then?
if DJANGO_VERSION >= (4, 2): | |
if (4, 2) <= DJANGO_VERSION < (5, 1): |
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.
That would work, but I think it might be more readable to reorder the cases so that DJANGO_VERSION >= (5, 1)
comes first, then elif DJANGO_VERSION >= (4, 2)
.
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.
👍 Done.
The commit history is very messy now. What would you prefer in this case? A new PR? Squashing/renaming commits?
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.
That's all fine, thanks! I'll deal with squashing the commits when I merge (which I'll do now - everything looks good).
Prevent "RemovedInDjango51Warning: The DEFAULT_FILE_STORAGE setting is deprecated. Use STORAGES instead."
Merged in 1fd4b86. Have credited you in the contributors list as |
Prevent "RemovedInDjango51Warning: The DEFAULT_FILE_STORAGE setting is deprecated. Use STORAGES instead."
Fixes #10461