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

Upgrade project to latest versions of python and django #560

Merged
merged 4 commits into from
Apr 29, 2023

Conversation

Alexerson
Copy link
Contributor

The main issue to fix is that the lib is currently not properly getting the default STORAGE backend if the project is already using the new 4.2 settings.

I also removed unsupported versions from tox and added the new ones.

@vstoykov
Copy link
Collaborator

Thank you for your contribution!

Can you also fix the .github/workflows/python.yml to use the same python versions as defined in the tox.ini?

@Alexerson
Copy link
Contributor Author

Alexerson commented Apr 27, 2023

Done!

Edit: actually let me try to add a test.

@Alexerson
Copy link
Contributor Author

Alexerson commented Apr 27, 2023

Done. Hopefully they pass properly (tox seems happy locally)… It’s a bit tricky to test settings because AppConf runs only once by default, but I think what I did really tests what’s happening.

@vstoykov vstoykov merged commit 3113ed8 into matthewwithanm:develop Apr 29, 2023
@vstoykov
Copy link
Collaborator

Thank you very much!

@Alexerson
Copy link
Contributor Author

Actually, I did some more digging and the way I fixed it, it won’t properly support OPTIONS for the new STORAGES key. As long as it’s just the BACKEND, all good, but OPTIONS are dropped. It might require a slightly more involved fix to do that properly. I’ll try to look into it.

@Alexerson
Copy link
Contributor Author

I think it will be hard to do this without actually dropping the current setting and replacing by a setting that look like the new Django one. Or alternatively, it could be a key that point to the STORAGES dict, but I’m not too sure if that’s standard.

Let me know if this is something you’d like me to try to look into. I might have some time by end of the week or early next week. Unfortunately, I don’t think it’s worth releasing what I did until then.

For now on my side, I’m setting cache_storage for each field separately to fix this.

@vstoykov
Copy link
Collaborator

vstoykov commented May 3, 2023

On a second look about the new config for storages it really seems that will be better to make IMAGEKIT_DEFAULT_FILE_STORAGE to be configured with the corresponding key in the STORAGES (when using DJango 4.2+)

Basically IMAGEKIT_DEFAULT_FILE_STORAGE = None and IMAGEKIT_DEFAULT_FILE_STORAGE = 'default' on DJango 4.2+ to be equivalent. This will work similarly as IMAGEKIT_CACHE_BACKEND. We can have only a fallback if the value is not in settings.STORAGES to try to load it as a class.

This behavior can be in the documentation and I think that will not be a problem. What you think. Do you will have time to propose new PR with these changes?

@vstoykov vstoykov mentioned this pull request May 3, 2023
@Alexerson
Copy link
Contributor Author

Sounds good. Do you really want to keep the same setting name, or would it make more sense to have a different setting name?
I’ll try to find the time to work on this on Monday (if not before). Thanks for your responsivity.

@Alexerson
Copy link
Contributor Author

See #561

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.

2 participants