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

Add Django 2.2 and 3.0 Support #300

Merged
merged 15 commits into from
Jan 11, 2020

Conversation

studybuffalo
Copy link

Major changes to allow Django 3.0 support

  • Switched import of python_2_unicode_compatible to the six library, as it is removed from Django.
  • Switched patch_logger (from Django) tests to assertLogs (from unittest), as patch_logger functionality is being dropped by Django.
  • Created a temporary test mixin that enables support of assertLogs in Python 2.7; once Python 2.7 support is dropped this mixin can be removed and all tests should continue to pass.
  • Pinned sorl-thumbnail to a specific commit that supports Django 1.11 to Django 3.0 - intent would be for this to be returned back to a released version as soon as the next release is made.

Other minor changes

  • Updated .travis.yml to remove unused statements and add recommended statements.

Relevant Issues: #291 #295 #296 #299

@studybuffalo
Copy link
Author

Question to be resolved: is it acceptable to continue with sorl-thumbnail pinned to the specific commit, or do we remove it as dependency as per issue #297

@coveralls
Copy link

coveralls commented Jan 8, 2020

Coverage Status

Coverage remained the same at 88.635% when pulling 4c52bb2 on studybuffalo:support-django-22-and-30 into 29f8208 on dokterbob:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.635% when pulling 5bafbdb on studybuffalo:support-django-22-and-30 into 29f8208 on dokterbob:master.

@dokterbob
Copy link
Collaborator

Great work! I'll try and review it ASAP.

Question to be resolved: is it acceptable to continue with sorl-thumbnail pinned to the specific commit, or do we remove it as dependency as per issue #297
I think pinning the version is good enough for now. There might be a future release of sorl-thumbnail - but removing the dependency is potentially a lot of work and until it's done this will allow users to stay using django-newsletter. :)

Thanks again!

@dokterbob
Copy link
Collaborator

Wow! Such a clean contribution! I've already reviewed it, there's only one comment, really:

Please add comments in the Travis file as well as in the requirements.txt about the pinned version of sorl-thumbnail, so that we're reminded to remove it once a new release comes out. I was also wondering why the dependency needs to be explicitly installed with Travis, while it's already in the requirements. That suggests it might create problems for users and/or require additional install instructions. Could you please verify whether it is necessary (to add this install step in Travis)?

Lastly, once the patch has been reviewed, I would appreciate if you could do a pipenv install -r requirements.txt to update the Pipenv file as well as the lockfile.

@studybuffalo
Copy link
Author

Thanks for catching that - I learned that requirements.txt editable dependencies don't work the same way for setuptools. I put the Travis file changes in to get things moving forward for development and then forgot about it.

I think I have got a simple enough fix working now that resolves those noted issues and we can easily pull it out once a new sorl-thumbnail version is released. I also made sure to flag it all with TODO comments.

With these changes passing CI, I have also gone and updated the lockfile as requested.

Copy link
Collaborator

@dokterbob dokterbob left a comment

Choose a reason for hiding this comment

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

I would request you find a way not to hardcode the line number in setup.py, please see comment.

setup.py Outdated Show resolved Hide resolved
studybuffalo added 3 commits January 11, 2020 07:03
Pinning to the specific 12.6.0 release commit
Removing "-e " from the DEPENDENCY_LINKS
@dokterbob dokterbob merged commit e4134e9 into jazzband:master Jan 11, 2020
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

3 participants