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

Replace get_storage_class with new storages API for Django >= 4.2 #1216

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

adnathanail
Copy link

In response to issue #1187 I've created a helper function _get_storage_class which detects whether to use the old or new API depending on the current Django version.

I've updated the code, and tests, to use this for now. Then when Django 4.1 support is dropped, it can be removed in favour of directly using storages.create_storage.

I also updated the docs about custom remote storages, so people know to use the new API if they are using Django >= 4.2

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.

This stops the error for me if I run the tests so...

python -Werror::PendingDeprecationWarning -m django test --settings=compressor.test_settings compressor

Comment on lines +19 to +20
from django.core.files.storage import get_storage_class
return get_storage_class(backend)()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should be using storages[backend] here (and likely override_settings) 🤔

(Rather than creating a new instance...)

Copy link
Author

Choose a reason for hiding this comment

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

I think currently storages[backend] wouldn't have anything in it?

Are we saying that users should be declaring a backend with compressor.storage.CompressorFileStorage for us to use, so we don't keep creating a new instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the whole storages thing is new so... 😜

I think if we were designing from scratch, we'd likely say something like, "If you declare a backend in STORAGES we'll use that, otherwise we'll automatically create one for you".

So we'd need a setting for the backend name, and then we could look for that on first access.

That's just a first thought. Open to suggestions.

@carltongibson
Copy link
Contributor

I wonder if we should be using storages[backend] here (and likely override_settings) 🤔

(Rather than creating a new instance...)

Compare #1202 from @PetrDlouhy which does it this way 🤔

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.

None yet

2 participants