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.

Sign up for GitHub

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

Jump to bottom

Added Django 3 Support #19

Closed
wants to merge 8 commits into from
Closed

Added Django 3 Support #19

wants to merge 8 commits into from

Conversation

tiagocordeiro
Copy link
Contributor

No description provided.

@klis87
Copy link
Owner

klis87 commented Dec 30, 2019

@tiagocordeiro Thank you so much for this! And apologies for my late response, it was Christmas time :)

I have several questions:

  1. Only Django 1.11 LTS supports Python 2, which won't be supported after April 2020, so maybe we could just get rid of Python 2 altogether? Then six package won't be needed at all then
  2. I see some tests commented, they stopped working or there is another reason for this?
  3. nice idea with Travis, I am afraid it won't work though as it won't have access to cloudinary credential vars, unless I am missing sth? Perhaps I will need to set them as env variables in Travis admin panel?
  4. there are other questions but I will ask them as review comments

klis87
klis87 requested changes Dec 30, 2019
View reviewed changes
.editorconfig Outdated
@@ -1,11 +0,0 @@
root = true
Copy link
Owner

Choose a reason for hiding this comment

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

could we pls revert this file back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

.travis.yml Outdated
install: pip install -r requirements-dev.txt codecov
script: tox
after_success:
- codecov
Copy link
Owner

Choose a reason for hiding this comment

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

based on my previous comment, maybe we could simplify this and get rid of python 2.7?
I never used travis with tox, do we need sth here extra to make cloudinary tests passing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use tox in travis simply add credentials as environment variables in project settings in travis

cloudinary_storage/app_settings.py Outdated
@@ -8,7 +8,7 @@
from django.core.exceptions import ImproperlyConfigured
from django.dispatch import receiver
from django.test.signals import setting_changed
from django.utils.six.moves import reload_module
from six.moves import reload_module
Copy link
Owner

Choose a reason for hiding this comment

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

we could simplify things a lot by ditching python 2, like those six imports

tests/tests/test_commands.py Outdated

@override_settings(STATICFILES_STORAGE='cloudinary_storage.storage.StaticHashedCloudinaryStorage')
@mock.patch.object(StaticHashedCloudinaryStorage, '_save')
class CollectStaticCommandWithHashedStorageTests(SimpleTestCase):
Copy link
Owner

Choose a reason for hiding this comment

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

could you please explain the reason for those changes? I would like to keep current coverage level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All tests that depend on that mock fail, for example those that check copied files etc. I'll try to fix it

requirements-dev.txt
@@ -0,0 +1,8 @@
tox>=2.3.1
Copy link
Owner

Choose a reason for hiding this comment

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

why we removed test_requirements.txt and added this file? this is about current naming convention standards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No special reason. Just because some tools like pyup.io use requirements * to check dependencies.
If you prefer we can go back as it was before.

@tiagocordeiro
Copy link
Contributor Author

Hi @klis87 , I apologize for the delay, I was away these days.
answering your first comment:

  1. I agree
  2. I had no success with these tests. There is a strange MagicMock behavior with Django 3, you can check it here: https://travis-ci.org/tiagocordeiro/django-cloudinary-storage/jobs/626727446
  3. To use tox in travis simply add credentials as environment variables in project settings in travis
  4. ok
    Ps.: I'm still crawling with Python / Django and mostly automated testing. My English doesn't help either, but I'll try.

@klis87
Copy link
Owner

klis87 commented Jan 14, 2020

@tiagocordeiro No worries, we do it in our free time anyway so no rush expected:)

  1. Please let me know if you solved it

Ps. Your English is fine, understood everything :)

@klis87
Copy link
Owner

klis87 commented Jan 30, 2020

@tiagocordeiro thx for updates. From the diff I guess that the only thing left is fixing tests for newest Django right?

@klis87 klis87 mentioned this pull request Feb 7, 2020
Closed
tiagocordeiro
tiagocordeiro commented Feb 7, 2020
View reviewed changes
tests/tests/test_commands.py
os.remove(manifest_path)
finally:
StaticHashedCloudinaryStorage.manifest_name = 'staticfiles.json'
# TODO: Make this test work in Django >= 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@klis87 I believe this is the last test I couldn't solve, I'll try again

Copy link

Choose a reason for hiding this comment

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

is it solved yet?

Thanks for your good work

@tomek-i
Copy link

tomek-i commented May 3, 2020

@tiagocordeiro why did this close? you decided to leave it as is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oyeolamilekan oyeolamilekan oyeolamilekan left review comments

@klis87 klis87 klis87 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tiagocordeiro @klis87 @tomek-i @oyeolamilekan